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

Apply configured service name mapping to DBM-injected dddbs #7064

Merged
merged 8 commits into from
Jun 5, 2024

Conversation

vandonr
Copy link
Contributor

@vandonr vandonr commented May 22, 2024

What Does This Do

The injected dddbs tag should respect service mapping.

For single statements, we create the span and inject the comment in the same instrumentation, so everything is well (code)
However, for prepared statements, it's more complicated, because it happens in 2 steps:

  • step 1: the call to prepare is instrumented, this is where we inject the comment, it's done in DBMCompatibleConnectionInstrumentation
  • step 2: the call to execute is instrumented, this is when the command is actually run, so this is where we create the span (this is done in AbstractPreparedStatementInstrumentation)
    Thus, we don't have the span yet when we do the injection for prepared statements.

Service mapping logic was only applied when a service name is set in a span:

public void setServiceName(final String serviceName) {
this.serviceName = trace.mapServiceName(serviceName);
this.topLevel = isTopLevel(parentServiceName, this.serviceName);
}

so if we get a service name not from a span, the mapping wouldn't be applied to it.

To fix this, I'm repeating the logic that is found here:

return traceConfig.getServiceMapping().getOrDefault(serviceName, serviceName);

@mcculls helped with the code to retrieve the current trace's config snapshot.

Jira ticket: APMS-12191

fixes #7034

@vandonr vandonr requested a review from a team as a code owner May 22, 2024 14:37
@amarziali
Copy link
Collaborator

Would it be possible to add tests?

@vandonr
Copy link
Contributor Author

vandonr commented May 23, 2024

Would it be possible to add tests?

yes I'll look into it. For now, it seems there might be some tests failing, that I cannot reproduce locally :(
I was hoping to draw inspiration from there.

Copy link
Collaborator

@amarziali amarziali left a comment

Choose a reason for hiding this comment

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

LGTM. I left some suggestions

@vandonr vandonr merged commit 23584d8 into master Jun 5, 2024
75 of 76 checks passed
@vandonr vandonr deleted the vandonr/dsm branch June 5, 2024 17:39
@github-actions github-actions bot added this to the 1.35.0 milestone Jun 5, 2024
@amarziali amarziali added the comp:database Database Monitoring label Jun 5, 2024
@PerfectSlayer PerfectSlayer changed the title apply configured service name mapping to DBM-injected dddbs Apply configured service name mapping to DBM-injected dddbs Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:database Database Monitoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JDBC connection service mapping not being used, and traceparent missing, in SQL comment
2 participants