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

docs: add observableGauge to the prometheus experimental example #4267

Conversation

Lp-Francois
Copy link
Contributor

@Lp-Francois Lp-Francois commented Nov 8, 2023

Which problem is this PR solving?

Lack of documentation for async functions and gauge observable regarding the prometheus experimental example.

Short description of the changes

  • Add an example with a promise returning a value and the createObservableGauge() function.
  • Add an image for the prometheus graph for the gauge metrics.

Type of change

improve documentation

How Has This Been Tested?

  • Ran locally the project with the changes, then went to http://localhost:9464 on chrome, and noticed the value changed for observable_gauge_requests at every reload.
Screenshot 2023-11-08 at 20 14 22

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

Signed-off-by: Francois LP <francois.le.pape@gmail.com>
@Lp-Francois Lp-Francois requested a review from a team November 8, 2023 19:26
Lp-Francois and others added 2 commits November 8, 2023 20:52
Signed-off-by: Francois LP <francois.le.pape@gmail.com>
Copy link

codecov bot commented Nov 9, 2023

Codecov Report

Merging #4267 (2a980f9) into main (df63272) will decrease coverage by 0.43%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4267      +/-   ##
==========================================
- Coverage   92.22%   91.79%   -0.43%     
==========================================
  Files         336      214     -122     
  Lines        9521     6547    -2974     
  Branches     2017     1465     -552     
==========================================
- Hits         8781     6010    -2771     
+ Misses        740      537     -203     

see 125 files with indirect coverage changes

Copy link
Member

@pichlermarc pichlermarc 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 for opening this PR.

I'm wondering if it wouldn't be better to just link to the documentation on metrics instrumentation from the readme. The observable gauge is not really prometheus specific and its behvaior does not vary depending on which exporter is used. 🤔

experimental/examples/prometheus/index.js Outdated Show resolved Hide resolved
from @pichlermarc

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
@Lp-Francois
Copy link
Contributor Author

Thank you for opening this PR.

I'm wondering if it wouldn't be better to just link to the documentation on metrics instrumentation from the readme. The observable gauge is not really prometheus specific and its behvaior does not vary depending on which exporter is used. 🤔

I did the PR in the first place because I was trying to move away from https://github.com/siimon/prom-client, so going directly to this repo was helpful to find an easy example on how opentelemtry js works for prometheus.
As gauge example was missing, I added it :)

@Lp-Francois
Copy link
Contributor Author

@pichlermarc any update? 👀

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

I see. I guess it wouldn't hurt to have the example here. 🙂
I think we can make it sync though for simplicity.

Comment on lines +54 to +57
observableGauge.addCallback(async observableResult => {
const value = await randomMetricPromise();
observableResult.observe(value, attributes);
});
Copy link
Member

Choose a reason for hiding this comment

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

hmm, why not observe the value synchronously here? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, why not observe the value synchronously here? 🤔

Often, when I had to observe a value at querying time, it was from an async process, so I just replicated what I often deal with :)

Copy link
Member

Choose a reason for hiding this comment

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

I agree that the ability to collect gauge asynchronously can be a good point to present in the example.

Comment on lines +54 to +57
observableGauge.addCallback(async observableResult => {
const value = await randomMetricPromise();
observableResult.observe(value, attributes);
});
Copy link
Member

Choose a reason for hiding this comment

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

I agree that the ability to collect gauge asynchronously can be a good point to present in the example.

@Lp-Francois
Copy link
Contributor Author

Lp-Francois commented Jan 16, 2024

Hey @pichlermarc , should we merge the PR now it is approved? :)

@pichlermarc pichlermarc merged commit 5afbcdb into open-telemetry:main Jan 25, 2024
17 checks passed
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
…n-telemetry#4267)

* docs: add observableGauge to the prometheus experimental example

Signed-off-by: Francois LP <francois.le.pape@gmail.com>

* docs: add gauge image to README

Signed-off-by: Francois LP <francois.le.pape@gmail.com>

* refactor: change comment wording

from @pichlermarc

Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>

---------

Signed-off-by: Francois LP <francois.le.pape@gmail.com>
Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
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.

4 participants