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 metrics example #2658

Merged
merged 8 commits into from
Dec 18, 2021

Conversation

svetlanabrennan
Copy link
Contributor

@svetlanabrennan svetlanabrennan commented Dec 8, 2021

Which problem is this PR solving?

This metrics example was referencing an older version of opentelemetry packages which was causing an error of TypeError: meter.createObservableGauge is not a function

Fixes # (issue)

Short description of the changes

Type of change

How Has This Been Tested?

Tested examples locally.

Checklist:

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

@svetlanabrennan svetlanabrennan requested a review from a team December 8, 2021 21:09
@dyladan
Copy link
Member

dyladan commented Dec 8, 2021

Thanks for taking on all these examples and docs. Its great to have them updated

@svetlanabrennan
Copy link
Contributor Author

@dyladan @vmarchaud There is one thing in this example that I wasn't sure about which was searching prometheus for "cpu_core_usage". I'm not seeing that option in the metrics explorer in prometheus. I can see other "process_cpu_seconds_total" but not "cpu_core_usage".

If you're able to shed some light on this, I'd appreciate that. Thanks!

@codecov
Copy link

codecov bot commented Dec 8, 2021

Codecov Report

Merging #2658 (c9471f3) into main (27bbb11) will decrease coverage by 0.36%.
The diff coverage is n/a.

❗ Current head c9471f3 differs from pull request most recent head 62eb14f. Consider uploading reports for the commit 62eb14f to get more accurate results

@@            Coverage Diff             @@
##             main    #2658      +/-   ##
==========================================
- Coverage   92.88%   92.52%   -0.37%     
==========================================
  Files         143      144       +1     
  Lines        5158     5177      +19     
  Branches     1102     1102              
==========================================
- Hits         4791     4790       -1     
- Misses        367      387      +20     
Impacted Files Coverage Δ
...emetry-core/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️
...-metrics-base/examples/metrics/metrics/observer.js 0.00% <0.00%> (ø)

@legendecas
Copy link
Member

The function signatures used in the example are the latest, not published WIP version in api-metrics. I'm not sure if the example should be reverted back to the previous versions. AFAICT, the examples should be updated with the api-metrics change.

People can always find the example that works with v0.27.0 at https://github.com/open-telemetry/opentelemetry-js/blob/experimental/v0.27.0/examples/metrics/metrics/observer.js.

@dyladan
Copy link
Member

dyladan commented Dec 9, 2021

Hmm @legendecas is right that the current example is actually more up to date than what is here, but I think it's confusing to have an examples directory where the examples don't work with the released features. The average user who comes to the repo looking for examples is going to expect them to work and be confused when they don't. I see a few options to move forward here:

  1. Move the examples which use the WIP metrics packages into the wip metrics package directory and use @svetlanabrennan's version in the root examples folder.
  2. Create a separate directory for WIP/dev examples/docs

@dyladan
Copy link
Member

dyladan commented Dec 10, 2021

@MSNev does your approval that you support the examples/ folder containing released versions and not WIP versions?

@MSNev
Copy link
Contributor

MSNev commented Dec 10, 2021

@MSNev Nev Wylie FTE does your approval that you support the examples/ folder containing released versions and not WIP versions?

Yes, the examples should be using the production versions as that is what (I assume / expect) new users will be using these for. It does mean that there will be a delay in "adding" new example features.

@dyladan
Copy link
Member

dyladan commented Dec 10, 2021

Do you think we should have a location for WIP examples? If so, where would you vote? I think I'd like to have the WIP examples directly in the package directory they are an example of.

@vmarchaud
Copy link
Member

I think we should remove the example, or move it to the wip sdk but that will require time to keep it up to date so i think removing it make more sense for now

@svetlanabrennan
Copy link
Contributor Author

I like the idea of moving it to a wip sdk. And if the wip examples are not working (from time to time because of updates/changes) then users will know it's because they're "wip" examples.

@dyladan
Copy link
Member

dyladan commented Dec 15, 2021

Can you handle that in this PR?

@svetlanabrennan
Copy link
Contributor Author

@dyladan This is completed.

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

A few things to fix the lint step. Otherwise looks good

svetlanabrennan and others added 4 commits December 17, 2021 09:12
…README.md

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
…README.md

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
@vmarchaud vmarchaud added the document Documentation-related label Dec 18, 2021
@vmarchaud vmarchaud merged commit 2c8601f into open-telemetry:main Dec 18, 2021
@svetlanabrennan svetlanabrennan deleted the update-metrics-example branch January 6, 2022 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
document Documentation-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants