-
Notifications
You must be signed in to change notification settings - Fork 212
@W-18759774 - Created opentelemetry.js file with utility functions to log OTel spans and metrics #2705
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
@W-18759774 - Created opentelemetry.js file with utility functions to log OTel spans and metrics #2705
Changes from 11 commits
17a3b2b
cae20be
682baf4
abbcf6d
c13f041
d3124b7
ea66f26
1afef73
9a1e0ec
2c54543
01da556
f6ffadf
0a83204
d344dc9
c68195a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,337 @@ | ||||||
| /* | ||||||
| * Copyright (c) 2025, Salesforce, Inc. | ||||||
| * All rights reserved. | ||||||
| * SPDX-License-Identifier: BSD-3-Clause | ||||||
| * For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause | ||||||
| */ | ||||||
|
|
||||||
| import {trace, context, SpanStatusCode} from '@opentelemetry/api' | ||||||
| import {hrTimeToMilliseconds, hrTimeToTimeStamp} from '@opentelemetry/core' | ||||||
| import logger from './logger-instance' | ||||||
|
|
||||||
| const DEFAULT_SERVICE_NAME = 'pwa-kit-react-sdk' | ||||||
|
|
||||||
| export const OTEL_CONFIG = { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's use the Remove the DEFAULT_SERVICE_NAME constant from opentelemetry-server.js:
And replace it with |
||||||
| serviceName: process.env.OTEL_SERVICE_NAME || DEFAULT_SERVICE_NAME, | ||||||
| enabled: process.env.OTEL_SDK_ENABLED === 'true', | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see we have an |
||||||
| b3TracingEnabled: process.env.OTEL_B3_TRACING_ENABLED === 'true' | ||||||
| } | ||||||
|
|
||||||
| export const getServiceName = () => OTEL_CONFIG.serviceName | ||||||
|
|
||||||
| const SERVICE_NAME = OTEL_CONFIG.serviceName | ||||||
|
||||||
| const SERVICE_NAME = OTEL_CONFIG.serviceName |
We don't need this line. As Adam mentioned, we can replace all instances of SERVICE_NAME with getServiceName()
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you agree with the previous comment we need to replace all references of SERVICE_NAME:
| 'service.name': SERVICE_NAME, | |
| 'service.name': getServiceName(), |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, we would replace it here:
| const tracer = trace.getTracer(SERVICE_NAME) | |
| const tracer = trace.getTracer(getServiceName()) |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need these 2 lines if ctx is not used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Do we need this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on this nit. It may not seem like a big deal that a change like this that doesn't do anything goes in the PR, but one downside is in the future if someone wants to do a git blame to see "hey who added this line about requiring jest.config here and why did they do that" then they will see this PR instead of the originating PR.