opentelemetry-js icon indicating copy to clipboard operation
opentelemetry-js copied to clipboard

feat: add the ability to set the views via the SDK constructor

Open weyert opened this issue 1 year ago • 13 comments

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

weyert avatar Jul 28 '22 23:07 weyert

Does anyone have a good idea how I can access the views of the meter provider to ensure they got passed in?

weyert avatar Jul 28 '22 23:07 weyert

Codecov Report

Merging #3124 (2931a15) into main (6807def) will decrease coverage by 0.02%. The diff coverage is 87.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:

codecov[bot] avatar Jul 28 '22 23:07 codecov[bot]

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.

dyladan avatar Jul 29 '22 13:07 dyladan

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.

Thank you for the tip. I have added the unit test :)

weyert avatar Jul 31 '22 14:07 weyert

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.

legendecas avatar Jul 31 '22 14:07 legendecas

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?

weyert avatar Jul 31 '22 15:07 weyert

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 :)

legendecas avatar Jul 31 '22 15:07 legendecas

@weyert what is the plan here? To update the tests to use the in memory exporter?

dyladan avatar Aug 03 '22 15:08 dyladan

If this is not ready for review please mark as draft, otherwise just comment and I will review it.

dyladan avatar Aug 03 '22 15:08 dyladan

@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 avatar Aug 04 '22 10:08 weyert

@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 avatar Aug 04 '22 11:08 pichlermarc

@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 :)

weyert avatar Aug 04 '22 17:08 weyert

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:

pichlermarc avatar Aug 05 '22 08:08 pichlermarc

Want to make sure @legendecas is happy since he had comments but this can be merged IMO

dyladan avatar Aug 16 '22 13:08 dyladan