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

feat: Add prometheus exporter #483

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Nov 4, 2019

Which problem is this PR solving?

With #482 this now provides an end-to-end metrics story

Short description of the changes

Adds a metrics exporter for the prometheus backend. In its current state, the exporter maintains an internal state with the values of all exported metrics, which is then returned when an endpoint is called by Prometheus.

@codecov-io
Copy link

codecov-io commented Nov 4, 2019

Codecov Report

Merging #483 into master will decrease coverage by 1.55%.
The diff coverage is 96.38%.

@@            Coverage Diff             @@
##           master     #483      +/-   ##
==========================================
- Coverage   92.03%   90.47%   -1.56%     
==========================================
  Files         138      144       +6     
  Lines        6264     7237     +973     
  Branches      527      640     +113     
==========================================
+ Hits         5765     6548     +783     
- Misses        499      689     +190
Impacted Files Coverage Δ
...pentelemetry-exporter-prometheus/src/prometheus.ts 91.83% <91.83%> (ø)
...emetry-exporter-prometheus/test/prometheus.test.ts 98.54% <98.54%> (ø)
...pentelemetry-core/test/internal/validators.test.ts 50% <0%> (-50%) ⬇️
...elemetry-core/test/trace/spancontext-utils.test.ts 55.55% <0%> (-44.45%) ⬇️
...lemetry-core/test/trace/ProbabilitySampler.test.ts 56.52% <0%> (-43.48%) ⬇️
...s/opentelemetry-core/test/trace/NoopTracer.test.ts 60% <0%> (-40%) ⬇️
...s/opentelemetry-core/test/context/B3Format.test.ts 63.39% <0%> (-36.61%) ⬇️
...ges/opentelemetry-core/test/trace/NoopSpan.test.ts 63.63% <0%> (-36.37%) ⬇️
...s/opentelemetry-core/test/trace/tracestate.test.ts 65.06% <0%> (-34.94%) ⬇️
...kages/opentelemetry-metrics/test/mocks/Exporter.ts 66.66% <0%> (-33.34%) ⬇️
... and 45 more

Copy link
Member

@mayurkale22 mayurkale22 left a comment

Choose a reason for hiding this comment

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

This is a great start, added a few comments in the first pass.

Co-Authored-By: Mayur Kale <[email protected]>
Copy link
Member

@mayurkale22 mayurkale22 left a comment

Choose a reason for hiding this comment

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

Thank you @dyladan for this PR and for addressing my feedback! LGTM!

* @param name name to be sanitized
*/
private _sanitizePrometheusMetricName(name: string): string {
return name.replace(this._invalidCharacterRegex, '_'); // replace all invalid characters with '_'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible this could cause duplicates? Should we write a warning in that case (could be a follow up task)?

Copy link
Member Author

@dyladan dyladan Nov 8, 2019

Choose a reason for hiding this comment

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

We could replace period with __DOT__ or similar

@mayurkale22
Copy link
Member

I managed to run the end to end example using prometheus exporter, looks like everything is working fine. I tried creating and updating simple counter and gauge metrics every 2 secs.

/metrics endpoint:
Screen Shot 2019-11-06 at 5 19 03 PM
On scraper:
Screen Shot 2019-11-06 at 5 20 10 PM

@mayurkale22 mayurkale22 merged commit 460d986 into open-telemetry:master Nov 11, 2019
@Flarna Flarna deleted the add-prometheus-exporter-dan branch December 13, 2019 20:58
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
* feat: add responseHook config to redis instrumentation

* fix: lint fix

* feat: add responseHook config to redis instrumentation

* fix: lint fix

* chore(deps): update all non-major dependencies (open-telemetry#483)

* chore: generalize the instrumentation file name (open-telemetry#479)

* feat: add responseHook config to graphql instrumentation

* Apply suggestions from code review

Co-authored-by: Bartlomiej Obecny <[email protected]>

* test: add a test for a responseHook that isn't a function

* fix: lint fix

Co-authored-by: WhiteSource Renovate <[email protected]>
Co-authored-by: Rauno Viskus <[email protected]>
Co-authored-by: Bartlomiej Obecny <[email protected]>
martinkuba pushed a commit to martinkuba/opentelemetry-js that referenced this pull request Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants