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

Update New Relic exporter. #3091

Merged

Conversation

a-feld
Copy link
Member

@a-feld a-feld commented Apr 13, 2021

Description: This updates the New Relic exporter.

Changes include:

  • Adds support for Logs
  • Updates to the configuration format (see example)
  • Removal of common attributes (use opentelemetry collector resource processor to add attributes)
  • Performance improvements
  • Optimizations to the New Relic payload to reduce payload size
  • Metrics generated for monitoring the exporter
  • Insert Key vs License keys are auto-detected in some cases
  • Collector version information is properly extracted via the application start info parameters

🚨 This PR drops support for cumulative metrics being sent to New Relic via a collector. 🚨

This is being done for the following reasons:

  1. Transforming a cumulative to delta metric is a stateful operation that requires careful routing consideration. Supporting cumulative metrics means that a customer would need to consider their collector deployment routing strategy.
  2. With the prior implementation, if data arrives out of order at the collector (which can happen for various reasons), the transform would display data that is potentially inaccurate (interval computation).
  3. The state held in the internal transformer was not persistent. It did not survive restarts and would be evicted if data was not observed for a period of 20 minutes. This could lead to missing data points.

We believe that solutions exist today, such as New Relic's prometheus scraper that currently work around these issues and provide a better experience.

Testing: Updated unit tests

Documentation: Updated readme

@a-feld a-feld force-pushed the newrelic-exporter-update branch 2 times, most recently from 4185eec to ba310b9 Compare April 13, 2021 13:53
@codecov
Copy link

codecov bot commented Apr 13, 2021

Codecov Report

Merging #3091 (1b44b29) into main (81dc441) will increase coverage by 0.13%.
The diff coverage is 98.50%.

❗ Current head 1b44b29 differs from pull request most recent head 4f44111. Consider uploading reports for the commit 4f44111 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3091      +/-   ##
==========================================
+ Coverage   91.59%   91.72%   +0.13%     
==========================================
  Files         486      487       +1     
  Lines       23519    23757     +238     
==========================================
+ Hits        21543    21792     +249     
+ Misses       1466     1457       -9     
+ Partials      510      508       -2     
Flag Coverage Δ
integration 63.35% <ø> (ø)
unit 90.73% <98.50%> (+0.14%) ⬆️

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

Impacted Files Coverage Δ
exporter/newrelicexporter/newrelic.go 96.23% <95.62%> (+2.00%) ⬆️
exporter/newrelicexporter/config.go 100.00% <100.00%> (ø)
exporter/newrelicexporter/factory.go 100.00% <100.00%> (ø)
exporter/newrelicexporter/metrics.go 100.00% <100.00%> (ø)
exporter/newrelicexporter/transformer.go 100.00% <100.00%> (+4.37%) ⬆️
internal/aws/metrics/metric_calculator.go
internal/aws/xray/util.go
internal/aws/xray/tracesegment.go
internal/awsxray/tracesegment.go 100.00% <0.00%> (ø)
internal/aws/metric_calculator.go 100.00% <0.00%> (ø)
... and 4 more

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 81dc441...4f44111. Read the comment docs.

@a-feld a-feld force-pushed the newrelic-exporter-update branch 5 times, most recently from a12dd3a to 04b2d5b Compare April 14, 2021 14:34
@a-feld a-feld marked this pull request as ready for review April 14, 2021 16:37
@a-feld a-feld requested a review from a team April 14, 2021 16:37
@bogdandrutu
Copy link
Member

@alanwest @a-feld @jack-berg @nrcventura please review :)

@a-feld
Copy link
Member Author

a-feld commented Apr 14, 2021

Thanks the team is looking, I think @nrcventura was going to add a bit more code coverage! I'll push this back into draft. Sorry about the premature "ready for review" 😄

@a-feld a-feld marked this pull request as draft April 14, 2021 18:12
@a-feld a-feld force-pushed the newrelic-exporter-update branch 2 times, most recently from 1b44b29 to f121e02 Compare April 15, 2021 16:13
@a-feld a-feld force-pushed the newrelic-exporter-update branch from f121e02 to 4f44111 Compare April 15, 2021 16:53
@a-feld
Copy link
Member Author

a-feld commented Apr 16, 2021

We've been discussing this PR at length within the team and we all agree that it's important to document some of the important breaking changes in this PR.

🚨 This PR drops support for cumulative metrics being sent to New Relic via a collector. 🚨

This is being done for the following reasons:

  1. Transforming a cumulative to delta metric is a stateful operation that requires careful routing consideration. Supporting cumulative metrics means that a customer would need to consider their collector deployment routing strategy.
  2. With the prior implementation, if data arrives out of order at the collector (which can happen for various reasons), the transform would display data that is potentially inaccurate (interval computation).
  3. The state held in the internal transformer was not persistent. It did not survive restarts and would be evicted if data was not observed for a period of 20 minutes. This could lead to missing data points.

We believe that solutions exist today, such as New Relic's prometheus scraper that currently work around these issues and provide a better experience.

@a-feld a-feld marked this pull request as ready for review April 16, 2021 20:18
@alanwest
Copy link
Member

@bogdandrutu This is good to go now.

@bogdandrutu bogdandrutu merged commit 765429a into open-telemetry:main Apr 19, 2021
pmatyjasek-sumo pushed a commit to pmatyjasek-sumo/opentelemetry-collector-contrib that referenced this pull request Apr 28, 2021
* Update New Relic exporter.

Co-authored-by: Jack Berg <jberg@newrelic.com>
Co-authored-by: Alan West <awest@newrelic.com>
Co-authored-by: Chris Ventura <cventura@newrelic.com>

* Fix lint errors.

* Add missing test.

* Add test coverage for api key value.

* Capture headers and validate key logic.

* Add unsupported metric type test.

* Add attribute metadata metric

* Update readme

* Fix speling.

* Add missing tests for metrics.go

* Update go.mod

* Add missing transformer.go tests

* Add missing tests for newrelic.go

* Fix lint error

Co-authored-by: Jack Berg <jberg@newrelic.com>
Co-authored-by: Alan West <awest@newrelic.com>
Co-authored-by: Chris Ventura <cventura@newrelic.com>
Co-authored-by: Chris Ventura <45495992+nrcventura@users.noreply.github.com>
Co-authored-by: Alan West <3676547+alanwest@users.noreply.github.com>
alexperez52 referenced this pull request in open-o11y/opentelemetry-collector-contrib Aug 18, 2021
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
@alanwest alanwest deleted the newrelic-exporter-update branch October 15, 2021 15:01
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.

6 participants