opentelemetry-js
opentelemetry-js copied to clipboard
feat: add the ability to set the views via the SDK constructor
Which problem is this PR solving?
Recently the ability to define views has been moved to the constructor of the MeterProvider
when you are using the node-sdk
you aren't not able to pass views anymore. This PR attempts to resolve this problem.
Fixes #3125 (issue)
Short description of the changes
Add the views: Views[]
-parameter to the configuration type
Type of change
Please delete options that are not relevant.
- [X] Bug fix (non-breaking change which fixes an issue)
- [X] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [X] This change requires a documentation update
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
- [x] Used yalc to verify it works
Checklist:
- [X] Followed the style guidelines of this project
- [x] Unit tests have been added
- [x] Documentation has been updated
Does anyone have a good idea how I can access the views
of the meter provider to ensure they got passed in?
Codecov Report
Merging #3124 (2931a15) into main (6807def) will decrease coverage by
0.02%
. The diff coverage is87.50%
.
@@ Coverage Diff @@
## main #3124 +/- ##
==========================================
- Coverage 93.21% 93.18% -0.03%
==========================================
Files 196 196
Lines 6414 6431 +17
Branches 1350 1359 +9
==========================================
+ Hits 5979 5993 +14
- Misses 435 438 +3
Impacted Files | Coverage Δ | |
---|---|---|
...imental/packages/opentelemetry-sdk-node/src/sdk.ts | 91.02% <87.50%> (-2.42%) |
:arrow_down: |
Does anyone have a good idea how I can access the
views
of the meter provider to ensure they got passed in?
You can access private properties using obj["_somePrivateProperty"]
syntax. I believe the views are in provider["_sharedState"].viewRegistry["_registeredViews"]
. You can also do provider["_sharedState"].viewRegistry.findViews(instrument, meter)
if you have access to the instrument.
Does anyone have a good idea how I can access the
views
of the meter provider to ensure they got passed in?You can access private properties using
obj["_somePrivateProperty"]
syntax. I believe the views are inprovider["_sharedState"].viewRegistry["_registeredViews"]
. You can also doprovider["_sharedState"].viewRegistry.findViews(instrument, meter)
if you have access to the instrument.
Thank you for the tip. I have added the unit test :)
Sorry for the delay, this is purely a suggestion: You can also test with the resulting metric streams with a MetricReader, like adding a renaming view and checking that the metric instrument is being renamed when exported, without accessing the internal states of the meter provider.
Sorry for the delay, this is purely a suggestion: You can also test with the resulting metric streams with a MetricReader, like adding a renaming view and checking that the metric instrument is being renamed when exported, without accessing the internal states of the meter provider.
I will give it shot :) If I understand you correctly you want me to test with like the in memory exporter?
If I understand you correctly you want me to test with like the in memory exporter?
Yeah, or more cleanly testing it with InMemoryExporter will be great. This is where InMemoryExporter is best suited :)
@weyert what is the plan here? To update the tests to use the in memory exporter?
If this is not ready for review please mark as draft, otherwise just comment and I will review it.
@weyert what is the plan here? To update the tests to use the in memory exporter?
Yes, that's the plan. I hope to sort it out before the end of the weekend. Only I am unable to switch the PR to draft
@weyert thanks for adding this and taking the time to switch over the tests. :rocket: I marked the PR as draft now, you should be able to mark it as ready at any point :slightly_smiling_face:
@pichlermarc I have updated the test, let me know, if this test with in memory exporter makes sense for you. If so, feel free to change the status of the PR to Ready for review :)
Looking good, thanks for adapting the tests. :slightly_smiling_face: I think this should be ready once the linting errors are addressed (there are a few unused imports and missing semicolons). :slightly_smiling_face:
Want to make sure @legendecas is happy since he had comments but this can be merged IMO