Reorg manual tracing JS docs based on SDK #1445
Conversation
cartermp
changed the title
|
I added comments on #1435 but reviewing two documents which both touch a lot of things is difficult to know which PR the comment "belongs" on. Some of those comments may be addressed here? |
| ``` | ||
|
|
||
| By default, the status for all spans is `Unset` rather than `Ok`. It is | ||
| typically the job of another component in your telemetry pipeline to interpret | ||
| the `Unset` status of a span, so it's best not to override this unless you're |
There was a problem hiding this comment.
This statement sounds to me like suggesting users to not set the status code OK. Am I understanding this correctly?
There was a problem hiding this comment.
Yep. It's at least my understanding that unless you're an instrumentation library author, you shouldn't set the status to Ok, but leave it as Unset for a processor or backend to interpret.
There was a problem hiding this comment.
Could you elaborate on why that is? Wouldn't the application dev know whether the span completed or not?
|
Okay, should be good for (hopefully final?) review. I moved the tracing init stuff to CJS modules. It seems to be the least headache to use right now in the sample apps I'm using as I document stuff. Once ESM is properly supported in the SDK we'll probably want to figure out a way to show both options and instructions for running. |
There was a problem hiding this comment.
Added a few comments, my main concerns are the following:
- by removing the example application it's much harder to follow the flow
- see my comment on
tracing.js, it's not clear why the code is separated out like for auto instrumentation (where a "don't touch my code" experience is nice). If the goal is to separate SDK & API this should be called out to not confuse the user.
Co-authored-by: Austin Parker <[email protected]>
|
@svrnm - this good to go? |
There are still 2 things open for me:
|
|
I appreciate the value of a sample app, but I think this PR is already pretty large and is already such an improvement over the existing document that it should be accepted and a sample can be taken as a follow up task. |
+1 @svrnm - your first point has been addressed, and I agree iwth @dyladan that further changes should be addresses separately. Might you be willing to approve this PR as is? |
@dyladan , @chalin : all I suggest is keeping the existing sample app: https://opentelemetry.io/docs/instrumentation/js/instrumentation/ But I don't want to block the PR, we can re-introduce it in a follow up. |
|
I'd also like to think about what a good sample looks like such that each subsection could be meaningfully applied in a way that doesn't feel arbitrary/strange. It might also be good to settle if we want to do something like that for all the manual instrumentation pages. Currently, none of them work off of a common sample/scenario. |
|
I closed my outstanding conversations as well, so we can move that "sample app" discussion to an issue for now. |
This is built on #1435 out of convenience, but I'm unopinionated on which order to pull in things.
Fixes #1436
I had to kind of reorg the manual instrumentation docs a bit for JS here. I also decided to pull in some text from other docs to clarify some sections, and linkify some concepts as well now that we have language-agnostic concepts.
Happy to break stuff out if that helps with review.
Since y'all aren't code owners yet, tagging @open-telemetry/javascript-approvers to take a look. I have no doubt that there may be a few technical inaccuracies here that I'd like to catch before merging 🙂
Preview: https://deploy-preview-1445--opentelemetry.netlify.app/docs/instrumentation/js/instrumentation/