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: fix examples in website_docs/instrumentation.md #2412

Merged
merged 3 commits into from Aug 14, 2021
Merged

docs: fix examples in website_docs/instrumentation.md #2412

merged 3 commits into from Aug 14, 2021

Conversation

svrnm
Copy link
Member

@svrnm svrnm commented Aug 12, 2021

Like open-telemetry/opentelemetry-js-api#116 this fixes the examples in the website docs to work properly again.

@svrnm svrnm requested a review from a team as a code owner August 12, 2021 19:46
@codecov
Copy link

codecov bot commented Aug 12, 2021

Codecov Report

Merging #2412 (eb6a770) into main (5b4ca1c) will increase coverage by 0.12%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2412      +/-   ##
==========================================
+ Coverage   92.49%   92.62%   +0.12%     
==========================================
  Files         134      137       +3     
  Lines        4850     4975     +125     
  Branches     1027     1048      +21     
==========================================
+ Hits         4486     4608     +122     
- Misses        364      367       +3     
Impacted Files Coverage Δ
...emetry-api-metrics/src/platform/node/globalThis.ts 0.00% <0.00%> (-100.00%) ⬇️
...sync-hooks/src/AbstractAsyncHooksContextManager.ts 97.01% <0.00%> (ø)
...async-hooks/src/AsyncLocalStorageContextManager.ts 100.00% <0.00%> (ø)
...ontext-async-hooks/src/AsyncHooksContextManager.ts 100.00% <0.00%> (ø)

@svrnm
Copy link
Member Author

svrnm commented Aug 13, 2021

This PR now also contains an update for the node.js example to use the node-sdk

@svrnm
Copy link
Member Author

svrnm commented Aug 13, 2021

A few open questions on that:

  • I didn't get it to work to use the NodeSDK for metrics. I guess there's still a few pieces missing?
  • The sdk.start() is in the required tracing.js file. Would it be better to add the code into the app.js and wait for the sdk.start() to fulfill before the express server is started?

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.

This looks way easier for a newcomer to follow thanks :)

@dyladan
Copy link
Member

dyladan commented Aug 13, 2021

A few open questions on that:

  • I didn't get it to work to use the NodeSDK for metrics. I guess there's still a few pieces missing?
  • The sdk.start() is in the required tracing.js file. Would it be better to add the code into the app.js and wait for the sdk.start() to fulfill before the express server is started?

I really want to make the sdk.start() be sync, but that requires removing the async nature of resource detection. I really don't know what to do there and the spec has been unhelpful each time i've tried to tackle it there.

@jasonkuhrt
Copy link

Nice! This lines up much more closely with what I've ended up having to do so far 🙏

@vmarchaud
Copy link
Member

I didn't get it to work to use the NodeSDK for metrics. I guess there's still a few pieces missing?

The current SDK is heavily behind the spec, speaking of this @dyladan do you have any news of the person of Dynatrace that was allocated time to work on metrics ?

@vmarchaud vmarchaud merged commit 33e6f71 into open-telemetry:main Aug 14, 2021
@vmarchaud vmarchaud added the document Documentation-related label Aug 14, 2021
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.

None yet

5 participants