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.

Sign up for GitHub

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

Jump to bottom

[frontend] document instrumentation #444

Merged
merged 5 commits into from
Oct 14, 2022
Merged

[frontend] document instrumentation #444

merged 5 commits into from
Oct 14, 2022

Conversation

puckpuck
Copy link
Contributor

Changes

  • Adds documentation for frontend, focused on instrumentation code.
  • Streamlines Instrumentation.js to be in line with how the payment service initializes the Node SDK.
  • Removed BackendTracer.js and moved code to InstrumentationMiddleware.js instead.

Some rationale for removing BackendTracer.js:

  • We have FrontendTracer.js which is used to initialize tracing in the browser, while BackendTracer was used to define utility tracing functions. These 2 different use cases with files of similar names can lead to confusion among users.
  • The utility functions in BackendTracer were only used once, making the move trivial.

@puckpuck puckpuck requested a review from a team October 14, 2022 05:31
@cartersocha
Copy link
Contributor

+1 Services table change

julianocosta89
julianocosta89 approved these changes Oct 14, 2022
View reviewed changes
Copy link
Member

@julianocosta89 julianocosta89 left a comment

Choose a reason for hiding this comment

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

really good one @puckpuck !

@cartersocha cartersocha merged commit c9a3018 into open-telemetry:main Oct 14, 2022
@puckpuck puckpuck deleted the docs.frontend branch October 14, 2022 06:19
mviitane
mviitane reviewed Oct 14, 2022
View reviewed changes
docs/services/frontend.md
It is recommended to use a Node required module when starting your NodeJS
application to initialize the SDK and auto-instrumentation. When initializing
the OpenTelemetry NodeJS SDK, you optionally specify which auto-instrumentation
libraries to leverage, or make use of the `getNodeAutoInstrumentations()`
Copy link
Member

Choose a reason for hiding this comment

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

Two options are described. It could be added that which one we selected for the demo app and why.

mviitane
mviitane reviewed Oct 14, 2022
View reviewed changes
docs/services/frontend.md
# frontend

The frontend is responsible to provide a UI for users, as well
as an API leveraged by the UI or other clients. The application is based on
Copy link
Member

Choose a reason for hiding this comment

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

After the first sentence, there could be a short, high-level description of the two parts of the instrumentation: browser side and the backend.

@mviitane
Copy link
Member

On my opinion, this PR got merged a bit too fast :)

@cartersocha
Copy link
Contributor

That’s fair! I’ll give it 24 hours next time.

But we’re also in crunch time and doc tweaks are an easy subsequent pr. Next week will hopefully be all focused on doc review with no code changes

@mviitane
Copy link
Member

That’s fair! I’ll give it 24 hours next time.

But we’re also in crunch time and doc tweaks are an easy subsequent pr. Next week will hopefully be all focused on doc review with no code changes

Also fair and makes sense to get the last code changes into the release.

jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
@puckpuck
[frontend] document instrumentation (open-telemetry#444)
5dd261c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mviitane mviitane mviitane left review comments

@julianocosta89 julianocosta89 julianocosta89 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@puckpuck @cartersocha @mviitane @julianocosta89