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

Instrumenting SDKs with open telemetry #69

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

JakeGinnivan
Copy link
Member

@JakeGinnivan JakeGinnivan commented Dec 10, 2023

Tests can export to OTEL if configured

image

@JakeGinnivan JakeGinnivan changed the title Instrumented js-sdk with open telemetry Instrumenting SDKs with open telemetry Dec 10, 2023
@JakeGinnivan JakeGinnivan marked this pull request as ready for review December 12, 2023 07:21
@@ -41,29 +45,18 @@ export function createBrowserClient({

environmentApiKey: string
}): BrowserClient {
const initialPromise = new PromiseCompletionSource<boolean>()
const tracer = getTracer()
Copy link
Member Author

Choose a reason for hiding this comment

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

This is probably the riskiest change in the PR.

I rewrite the retry logic and removed a bunch of duplication in this function based on the traces not quite looking right.

import { http, passthrough } from 'msw'

export const openTelemetryTracePassthrough = http.post(
'http://localhost:4318/v1/traces',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this meant to be hardcoded?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's just for unit tests if you configure open telemetry locally using Jaeger it will pass the trace calls through.

This wouldn't work if you wanted to use honeycomb or app insights for unit test telemetry though

externalStateStore,
)
if (process.env.FEATUREBOARD_SDK_DEBUG) {
stateStore = new DebugFeatureStateStore(stateStore)
Copy link
Member Author

Choose a reason for hiding this comment

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

I used a decorator pattern to reduce overhead when not debugging

@@ -21,6 +21,7 @@
"executor": "nx:run-commands",
"options": {
"commands": [
"echo \"export const version = '$(jq -r '.version' package.json)';\" > src/version.ts",
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this going to cause issues when you publish as there will be non committed changes in git.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've run the command before we do the commit, so the version number update should get checked in.

docs/use-verdaccio.md Outdated Show resolved Hide resolved
return initialisedState.initialisedPromise.completed
const err = resolveError(error)

initializeSpan.setStatus({
Copy link
Contributor

Choose a reason for hiding this comment

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

Already set inside retry when fail, do we need to set it on this span as well?

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