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

feat: add semantic browser resource fields & honeycomb distro info #23

Merged
merged 6 commits into from
Jan 22, 2024

Conversation

ahrbnsn
Copy link
Contributor

@ahrbnsn ahrbnsn commented Jan 17, 2024

Which problem is this PR solving?

Short description of the changes

Swaps out the browserDetection export from @opentelemetry/resources, replaces it with the one provided by @opentelemetry/opentelemetry-browser-detector

How to verify that this has the expected result

Run the example app. For maximum fields, use a Chromium browser. In honeycomb, queries for the new fields should show your events:

  • browser.platform: Chromium only
  • browser.brands: Chromium only
  • browser.mobile: Chromium only
  • browser.language
  • browser.user_agent: note: this doesn't match the otel semantic conventions ofbrowser.user_agent.original
  • honeycomb.distro.version: expected value: 0.0.11
  • honeycomb.distro.runtime_version: expected value browser

@ahrbnsn ahrbnsn requested a review from a team as a code owner January 17, 2024 20:05
@@ -42,6 +41,8 @@ import {
import { SemanticResourceAttributes } from '@opentelemetry/semantic-conventions';
import { WebSDKConfiguration } from './types';
import { SessionIdSpanProcessor } from './session-id-span-processor';
import { browserDetector } from '@opentelemetry/opentelemetry-browser-detector';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this puts the useragent in a browser.user_agent field instead of the documented browser.user_agent.original field — should we open an upstream PR to adjust, or fork the package?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should flag this upstream! In the meantime, when we do anything with this field in Honeycomb we should support both.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW this is already being investigated upstream as all of the semantic conventions are out of date. I would agree though that we should support both.

src/version.ts Outdated
@@ -0,0 +1 @@
export const VERSION = '0.0.11';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

at $PREV_JOB we had a build step that automatically bumped the version number when publishing a package, so i went with that but unsure if that's how we want to roll

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep that works! We'll need this for the release process 💯 . Curious where we've got 0.0.11 from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the deranged vapors of my mind seeing 0.0.1 & reading 0.0.10 — i'll update to 0.0.2?

@@ -42,6 +41,8 @@ import {
import { SemanticResourceAttributes } from '@opentelemetry/semantic-conventions';
import { WebSDKConfiguration } from './types';
import { SessionIdSpanProcessor } from './session-id-span-processor';
import { browserDetector } from '@opentelemetry/opentelemetry-browser-detector';
Copy link
Contributor

Choose a reason for hiding this comment

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

We should flag this upstream! In the meantime, when we do anything with this field in Honeycomb we should support both.

src/base-otel-sdk.ts Outdated Show resolved Hide resolved
src/version.ts Outdated
@@ -0,0 +1 @@
export const VERSION = '0.0.11';
Copy link
Contributor

Choose a reason for hiding this comment

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

Yep that works! We'll need this for the release process 💯 . Curious where we've got 0.0.11 from?

Base automatically changed from purvi/basic-config to main January 19, 2024 16:44
@ahrbnsn ahrbnsn force-pushed the ahr/browser-resource-attributes branch from 409b752 to 24ec8a5 Compare January 19, 2024 16:59
@ahrbnsn ahrbnsn marked this pull request as draft January 19, 2024 17:02
@ahrbnsn ahrbnsn marked this pull request as ready for review January 19, 2024 18:48
@ahrbnsn ahrbnsn requested a review from pkanal January 19, 2024 18:48
Copy link
Contributor

@pkanal pkanal left a comment

Choose a reason for hiding this comment

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

YAY ✅

@ahrbnsn
Copy link
Contributor Author

ahrbnsn commented Jan 22, 2024

my editor has a plugin that's doing some autoformatting on the markdown tables that is just havoc - removing my changes to the readme so i can merge this PR. will do a followup w/ readme update once i track down the culprit and disable it

@ahrbnsn ahrbnsn merged commit 55dddd3 into main Jan 22, 2024
7 checks passed
@ahrbnsn ahrbnsn deleted the ahr/browser-resource-attributes branch January 22, 2024 15:18
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.

Add browser resource attributes
3 participants