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(tracer): fix tag setting on span #1208

Merged
merged 1 commit into from
Mar 13, 2024
Merged

fix(tracer): fix tag setting on span #1208

merged 1 commit into from
Mar 13, 2024

Conversation

seaerchin
Copy link
Contributor

@seaerchin seaerchin commented Mar 13, 2024

Problem

we were setting the tags for the trace as a bootstrap process. this means that the tracer.init call does not bootstrap properly (as tracer.use is meant to be called at start-up, prior to actual tracing).

this PR changes it so that the tags are set on the span.

Solution

  1. instead of calling tracer.use, which is a bootstrap call, change to get the active span and calling setTag on that span.

Tests

  • Find traces in isomer's APM like this one, and verify that the tags are present

@seaerchin seaerchin requested review from a team and timotheeg March 13, 2024 07:09
Copy link
Contributor

@timotheeg timotheeg left a comment

Choose a reason for hiding this comment

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

I suppose we can merge this first since it's meant to be equivalent as what was there before, just cleaner.

But it's still weird to add tags to the scope's span, when we are in the context of a specific axios call.

Since a request handled by express can make multiple axios calls, typically we want to:

  1. track the request user itself once at the level of the request's scope, and
  2. if we need to track information at the level of the individual axios calls, then they should have their own spans, which would carry their own tags. That, or have log entries with enough structured data.

For 1. there's even a tracer.setUser() utility for that

@timotheeg timotheeg merged commit 65ab320 into develop Mar 13, 2024
19 checks passed
@timotheeg timotheeg deleted the fix/tracer branch March 13, 2024 09:17
@dcshzj dcshzj mentioned this pull request Mar 14, 2024
14 tasks
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.

2 participants