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 instrumentation.provider to newrelic default attrs #2900

Merged
merged 2 commits into from
Mar 29, 2021

Conversation

gunturaf
Copy link
Contributor

Description:
Add instrumentation.provider = opentelemetry to exported attributes

Link to tracking Issue: #2621

Testing: updated newrelic_test.go and transformer_test.go

Documentation:

@gunturaf gunturaf requested a review from a team March 27, 2021 12:06
@codecov
Copy link

codecov bot commented Mar 27, 2021

Codecov Report

Merging #2900 (df249f7) into main (88782b0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2900   +/-   ##
=======================================
  Coverage   91.54%   91.54%           
=======================================
  Files         463      463           
  Lines       22779    22780    +1     
=======================================
+ Hits        20853    20854    +1     
  Misses       1435     1435           
  Partials      491      491           
Flag Coverage Δ
integration 69.09% <ø> (ø)
unit 90.51% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
exporter/newrelicexporter/transformer.go 95.62% <100.00%> (+0.02%) ⬆️

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 88782b0...df249f7. Read the comment docs.

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just small comment, thanks

"collector.name": name,
"collector.version": version,
"name": "root",
instrumentationProviderAttrKey: "opentelemetry",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would stick with string to make the test easier to read without having to double check against the code. Otherwise, let's update the other keys to use the variables - mixing strings and variables looks a bit odd.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I'll revert the key to string.

@a-feld
Copy link
Member

a-feld commented Mar 29, 2021

Hi @gunturaf , thanks for the PR! I'm checking with our UI folks right now. We were supposed to have removed the requirement on instrumentation.provider so I'm not sure why this would cause UI issues. Will post an update as soon as I have one! ❤️

@bogdandrutu bogdandrutu reopened this Mar 29, 2021
@a-feld
Copy link
Member

a-feld commented Mar 29, 2021

Yup looks like it's currently needed.

@bogdandrutu bogdandrutu merged commit 8e7fa51 into open-telemetry:main Mar 29, 2021
pmatyjasek-sumo pushed a commit to pmatyjasek-sumo/opentelemetry-collector-contrib that referenced this pull request Apr 28, 2021
…y#2900)

* add instrumentation.provider to newrelic default attrs

* use string instead of variables for keys in test
alexperez52 referenced this pull request in open-o11y/opentelemetry-collector-contrib Aug 18, 2021
Allows Summary metrics to be exported to Prometheus.
To walk through this feature, I created an adaptation of a Brian Brazil
tutorial for DropWizard, to create a Java server at
https://github.com/odeke-em/bugs/tree/master/opentelemetry-collector/2661
and it uses DropWizard, and exports out JVM statistics with Prometheus where the
*gc_collection_seconds are of the summary kind when scraped by visiting
http://localhost:1234/metrics which produced

```
 # HELP jvm_gc_collection_seconds Time spent in a given JVM garbage collector in seconds.
 # TYPE jvm_gc_collection_seconds summary
 jvm_gc_collection_seconds_count{gc="G1 Young Generation",} 4.0
 jvm_gc_collection_seconds_sum{gc="G1 Young Generation",} 0.026
 jvm_gc_collection_seconds_count{gc="G1 Old Generation",} 0.0
```

and then roundtripped the collector to scrape those metrics and then
export them to Prometheus

    VS

making Prometheus directly scrape that endpoint and then comparing
results.

Also added an end to end test to ensure that a mock DropWizard server
which produces JVM statistics can be scraped by the Prometheus receiver,
which will feed metrics to an active Prometheus exporter, and then we
scrape from the Prometheus exporter to ensure that the summary metrics
are written out and that they make sense.

    DropWizard -> Prometheus Receiver -> Prometheus Exporter -> HTTP scrape + Verify

Fixes #2661
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.

4 participants