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

maint(deps): bump the otel group in /packages/honeycomb-opentelemetry-web with 8 updates #349

Merged
merged 3 commits into from
Nov 7, 2024
Merged

maint(deps): bump the otel group in /packages/honeycomb-opentelemetry-web with 8 updates #349

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bb3f9ec
maint(deps): bump the otel group
dependabot[bot] Nov 4, 2024
4636f85
OTLPTraceExportor headers now must be Record<string, string>
MustafaHaddara Nov 5, 2024
0a6f75f
include note in changelog so it shows up in the next release
MustafaHaddara Nov 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions packages/honeycomb-opentelemetry-web/src/http-json-trace-exporter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,18 @@ export function configureHoneycombHttpJsonTraceExporter(
const apiKey = getTracesApiKey(options);
return new OTLPTraceExporter({
url: getTracesEndpoint(options),
headers: {
[TEAM_HEADER_KEY]: apiKey,
[DATASET_HEADER_KEY]: isClassic(apiKey) ? options?.dataset : undefined,
...options?.headers,
},
headers: configureHeaders(options, apiKey),
});
}

function configureHeaders(options?: HoneycombOptions, apiKey?: string) {
const headers = { ...options?.headers };
if (apiKey && !headers[TEAM_HEADER_KEY]) {
headers[TEAM_HEADER_KEY] = apiKey;
}
if (isClassic(apiKey) && options?.dataset) {
headers[DATASET_HEADER_KEY] = options?.dataset;
}
Comment on lines +25 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

OTLPTraceExporter updated their headers type so that it is Record<string, string>, which means values can't be undefined anymore.

This code change now only adds the values if they exist.


return headers;
}
2 changes: 1 addition & 1 deletion packages/honeycomb-opentelemetry-web/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export interface HoneycombOptions extends Partial<WebSDKConfiguration> {
endpoint?: string;

/** Optionally pass extra headers to the exporter. Commonly used if sending to a collector that requires authentication */
headers?: { [key: string]: string | number };
headers?: { [key: string]: string };
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically this is a breaking change 😭

The alternative is to map over options.headers in the configureHeaders function above and stringify any numeric values we get.

That said, I would rather we stick to the otel API as closely as possible.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I guess were still labeled as experimental but also we have this in the readme

Data shapes are stable and safe for production

If its not too hard then massaging the values as we are already doing a little seems ok.

But also I think it's fine to have this breaking change and call it out in the release notes. Maybe point fingers at the otel package that caused it as it looks like none of the packages increment a major version number for this

Copy link
Contributor

Choose a reason for hiding this comment

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

Under semver, if you're still 0.x, breaking changes can be included in a change in the minor version (ie. 0.84 can contain breaking changes from version 0.83, which is exactly what happened in this PR and captured in their changelog here.

I'll merge this change as-is and link back to the OTel change. I'm also opening an issue against otel-js to update their changelog.

Copy link
Contributor

Choose a reason for hiding this comment

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

open-telemetry/opentelemetry-js#5125


/** The API endpoint where traces telemetry is sent. Defaults to endpoint if not set. */
tracesEndpoint?: string;
Expand Down