Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add spanmetricsprocessor #1914

Closed
wants to merge 1 commit into from
Closed

Conversation

albertteoh
Copy link
Contributor

Signed-off-by: albertteoh albert.teoh@logz.io

Description:

Adds a new processor for aggregating span data to produce Requests Error and Duration (R.E.D) metrics.

Note: as advised by maintainers during the Agent/Collector SIG, a workaround configuration of adding a dummy receiver and pipeline is required due to current limitations of the pipeline design and constraints.

Link to tracking Issue: #403

Testing:

  • Unit tests for config, factory and processor logic with 100% line coverage.
  • Integration tested locally with Prometheus scraping and visualizing in Grafana.
    • Successfully produced metrics into our production environment as a Proof of Concept, using the prometheusremotewrite exporter.

Documentation:

  • Add README.md with usage instructions and example config.
  • Additional example configuration for different scenarios with detailed comments.
  • Comments to config.go for detailed configuration instructions.

Signed-off-by: albertteoh <albert.teoh@logz.io>
@albertteoh albertteoh requested a review from a team December 28, 2020 06:46
@albertteoh
Copy link
Contributor Author

Apologies in advance for the largish diff; I thought it would be more useful for reviewers to have the full context rather a portion of the picture. Most of the diff is composed of tests, sample/test config, generated mocks and documentation.

However, if it would help reviewers to reduce the diff size, please advise how you wish each partial diff to be structured and I can close this PR and create the smaller diffs as advised.

@codecov
Copy link

codecov bot commented Dec 28, 2020

Codecov Report

Merging #1914 (bcd2869) into master (c228239) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1914      +/-   ##
==========================================
+ Coverage   89.95%   90.04%   +0.09%     
==========================================
  Files         381      384       +3     
  Lines       18519    18698     +179     
==========================================
+ Hits        16659    16837     +178     
- Misses       1390     1391       +1     
  Partials      470      470              
Flag Coverage Δ
integration 69.84% <0.00%> (+0.01%) ⬆️
unit 88.78% <100.00%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/otelcontribcol/components.go 86.30% <100.00%> (+0.19%) ⬆️
processor/spanmetricsprocessor/factory.go 100.00% <100.00%> (ø)
processor/spanmetricsprocessor/json.go 100.00% <100.00%> (ø)
processor/spanmetricsprocessor/processor.go 100.00% <100.00%> (ø)
processor/groupbytraceprocessor/event.go 95.96% <0.00%> (-0.81%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c228239...bcd2869. Read the comment docs.

@gramidt
Copy link
Member

gramidt commented Dec 28, 2020

Exciting work, @albertteoh! I can't wait to test it out.

Typically when adding new components, the following PR order is recommended in the contributing docs.

  • First PR should include the overall structure of the new component:
    • Readme, configuration, and factory implementation usually using the helper factory structs.
    • This PR is usually trivial to review, so the size limit does not apply to it.
  • Second PR should include the concrete implementation of the component.
    If the size of this PR is larger than the recommended size consider splitting it in multiple PRs.
  • Last PR should enable the new component and add it to the otelcontribcol binary by updating the components.go file.
    The component must be enabled only after sufficient testing, and there is enough confidence in the stability and quality of the component.

@albertteoh
Copy link
Contributor Author

Thanks @gramidt! Closing this PR and will start with a new PR containing the overall structure as advised.

@albertteoh albertteoh closed this Dec 28, 2020
dyladan referenced this pull request in dynatrace-oss-contrib/opentelemetry-collector-contrib Jan 29, 2021
ljmsc referenced this pull request in ljmsc/opentelemetry-collector-contrib Feb 21, 2022
…1914)

Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.37.0 to 1.37.1.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.37.0...v1.37.1)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants