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

Add a warning to browser getting started #3125

Merged

Conversation

svrnm
Copy link
Member

@svrnm svrnm commented Aug 2, 2023

We have the same warning now for Manual instrumentation - Browser, I think the getting started should have it as well, because at least I get ~weekly questions around OTel for browser/mobile/etc.

@svrnm svrnm requested review from a team August 2, 2023 12:45
Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Oh, sorry, I missed that warning in the manual page earlier, but now it makes me want to ask: Why is the OTel JS status not reflecting this "experimental" status?

@svrnm
Copy link
Member Author

svrnm commented Aug 2, 2023

Oh, sorry, I missed that warning in the manual page earlier, but now it makes me want to ask: Why is the OTel JS status not reflecting this "experimental" status?

Because browser and nodejs are different here. Good point, maybe we should also add some clarity on that page.

@pichlermarc
Copy link
Member

Oh, sorry, I missed that warning in the manual page earlier, but now it makes me want to ask: Why is the OTel JS status not reflecting this "experimental" status?

Because browser and nodejs are different here. Good point, maybe we should also add some clarity on that page.

I fully agree. We should also do it in the OTel JS repo's README, as it could be clearer from that one too. I opened an issue on the JS repo to tackle that: open-telemetry/opentelemetry-js#4035

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Ok, I'd vote to clarify the status on the JS landing page first.

If we do add a warning, it should be short -- especially if you're wanting to repeat the warning across several pages (maybe we need a shortcode to help keep things DRY)?

See inline comment.

@chalin
Copy link
Contributor

chalin commented Aug 2, 2023

Regarding the JS status: maybe it should be "mixed" with a footnote clarifying the status?

@svrnm
Copy link
Member Author

svrnm commented Aug 3, 2023

I added the note to the hugo.yaml similar to the one we have for docker compose. I use the same wording now on three places: _index, getting-started and manual.

I also rephrased it once again to the following and decided against updating the status table of OTel JS:

Client instrumentation for the browser is experimental and mostly unspecified. If you are interested in helping out, get in touch with the Client Instrumentation SIG

Some rational for that: Upading the status table would be confusing, since the API & SDK for JavaScript in general are following the specification for traces, metrics, logs, etc. What we are looking at here is different from that as it is for Client-side instrumentation (or real user monitoring) which is (a) not solely a problem of the JS community and (b) will likely lead to an additional set of API/SDK specs or make use of existing ones in a certain way.

I took the wording "mostly unspecified" from the [Roadmap - P2: Client Instrumentation (RUM)]:

OpenTelemetry JS has technically supported capturing spans from web browsers since its first releases, however this behavior was mostly unspecified

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Well done with the refactoring. LGTM ✨

@chalin chalin force-pushed the add-warning-to-browser-getting-started branch from 220c753 to 4a66cd2 Compare August 3, 2023 17:02
svrnm and others added 3 commits August 3, 2023 13:28
Signed-off-by: svrnm <neumanns@cisco.com>
Co-authored-by: Patrice Chalin <chalin@users.noreply.github.com>
Signed-off-by: svrnm <neumanns@cisco.com>
@chalin chalin force-pushed the add-warning-to-browser-getting-started branch from 4a66cd2 to f3935c2 Compare August 3, 2023 17:28
@cartermp cartermp merged commit 249f954 into open-telemetry:main Aug 3, 2023
13 checks passed
@@ -171,6 +171,9 @@ params:
docker-compose-v2: |
`docker-compose` is deprecated. For details, see
[Migrate to Compose V2](https://docs.docker.com/compose/migrate/).
browser-instrumentation: |
Client instrumentation for the browser is **experimental** and mostly **unspecified**. If you are interested in
helping out, get in touch with the [Client Instrumentation SIG](https://docs.google.com/document/d/16Vsdh-DM72AfMg_FIt9yT9ExEWF4A_vRbQ3jRNBe09w/edit)
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, missed that: this sentence should end with a period.

Copy link
Member Author

Choose a reason for hiding this comment

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

let's have a follow up PR then for the period.

Copy link
Contributor

Choose a reason for hiding this comment

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

See #3134.

@svrnm svrnm deleted the add-warning-to-browser-getting-started branch November 23, 2023 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants