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

fix(otel): Add transaction source logic to otel spans #6160

Merged
merged 1 commit into from
Nov 9, 2022

Conversation

AbhiPrasad
Copy link
Member

@AbhiPrasad AbhiPrasad self-assigned this Nov 8, 2022
@AbhiPrasad AbhiPrasad added this to the OpenTelemetry Support milestone Nov 8, 2022
@@ -462,6 +462,36 @@ describe('SentrySpanProcessor', () => {
});
});

it('adds transaction source `url` for HTTP_TARGET', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

l: Instead of adding these tests, maybe it is better to adjust all the tests starting here:

it('updates op/description based on attributes for HTTP_METHOD without HTTP_ROUTE', async () => {
to simply also check the source? There are already tests for the different otel attribute variations, IMHO we could just add an expect(sentrySpan?.transaction?.metadata.source).toBe('XXX'); check to all of them?

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 did this because source only applies for transactions - so I wanted an explicit scenario where we were checking the transaction (span.transaction when it's a transaction is a self reference) for this test. Gonna keep it like this for now, we can always update later on if need be.

Copy link
Member

@mydea mydea left a comment

Choose a reason for hiding this comment

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

LGTM, imho maybe we can restructure the added tests and instead add the check for source to the existing tests of the different scenarios of attributes, but I also don't have strong feelings about this!

@AbhiPrasad AbhiPrasad merged commit 8214839 into master Nov 9, 2022
@AbhiPrasad AbhiPrasad deleted the abhi-otel-transaction-source branch November 9, 2022 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants