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

airbyte-metrics/server: add tags to root span from OAuth handler #20468

Merged
merged 4 commits into from
Dec 15, 2022
Merged

airbyte-metrics/server: add tags to root span from OAuth handler #20468

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
18c616d
add addTagsToRootSpan
alafanechere Dec 13, 2022
a58cbbf
call addTagsToRootSpan in OauthHandler
alafanechere Dec 14, 2022
46f54ea
testAddingTagsToRootSpan
alafanechere Dec 15, 2022
ec462dc
test adding error to root span
alafanechere Dec 15, 2022
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
15 changes: 15 additions & 0 deletions airbyte-metrics/metrics-lib/src/main/java/io/airbyte/metrics/lib/ApmTraceUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,21 @@ public static void addExceptionToTrace(final Span span, final Throwable t) {
}
}

/**
* Adds all the provided tags to the root span.
*
* @param tags A map of tags to be added to the root span.
*/
public static void addTagsToRootSpan(final Map<String, Object> tags) {
final Span activeSpan = GlobalTracer.get().activeSpan();
if (activeSpan instanceof MutableSpan) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[question] If the framework is wrapping all requests in a span, how would we not have a span? Perhaps if Data dog is disabled?

Copy link
Contributor

Choose a reason for hiding this comment

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

@evantahler Good catch. This method should check if activeSpan is not null. It will be null when the DataDog Java agent is not present. cc: @alafanechere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. Isn't if activeSpan instanceof MutableSpan already checking a not null by itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@evantahler MutableSpan is a datadog interface. And Span and GlobalTracer.get().activeSpan() are from opentelemetry. So when datadog is disabled we might not received Span object implementing the MutableSpan interface, hence the importance of this check. Am I right @jdpgrailsdev ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@alafanechere Correct on both accounts. The instanceof check does protect us from null, as it will resolve to false. MutableSpan is a DataDog only class, so if a customer uses a different APM product that supports OpenTracing, that check protects us from calling methods that don't exist on the object.

final MutableSpan localRootSpan = ((MutableSpan) activeSpan).getLocalRootSpan();
tags.entrySet().forEach(entry -> {
localRootSpan.setTag(formatTag(entry.getKey(), TAG_PREFIX), entry.getValue().toString());
});
}
}

/**
* Adds an exception to the root span, if an active one exists.
*
Expand Down
6 changes: 4 additions & 2 deletions airbyte-server/src/main/java/io/airbyte/server/handlers/OAuthHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,10 @@ public OAuthHandler(final ConfigRepository configRepository,

public OAuthConsentRead getSourceOAuthConsent(final SourceOauthConsentRequest sourceOauthConsentRequest)
throws JsonValidationException, ConfigNotFoundException, IOException {
ApmTraceUtils.addTagsToTrace(Map.of(WORKSPACE_ID_KEY, sourceOauthConsentRequest.getWorkspaceId(), SOURCE_DEFINITION_ID_KEY,
sourceOauthConsentRequest.getSourceDefinitionId()));
final Map<String, Object> traceTags = Map.of(WORKSPACE_ID_KEY, sourceOauthConsentRequest.getWorkspaceId(), SOURCE_DEFINITION_ID_KEY,
sourceOauthConsentRequest.getSourceDefinitionId());
ApmTraceUtils.addTagsToTrace(traceTags);
ApmTraceUtils.addTagsToRootSpan(traceTags);
Copy link
Contributor

Choose a reason for hiding this comment

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

[non-blocking nit] when I've done instrumentation like this in the past, the strategy was to append as much information to the trace / root-span at the top level before getting into any individual controller. This looks like a global rootSpanHelper which intercepts /every/ request (e.g. "request middleware") and:

  • if there's a user, addd the user_id
  • if there's a workspace, add the workspace_id
  • if there are any LaunchDarkly feature flags, add them to the span

Then, in the controller itself, you only have to add new information (the source_id in this case).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this idea of helper/request middleware. This abstraction could also help us switch tracking backend (sentry/datadog) with a single interface. @evantahler could it fit a bigger project we'd have in Q1?

final StandardSourceDefinition sourceDefinition =
configRepository.getStandardSourceDefinition(sourceOauthConsentRequest.getSourceDefinitionId());
final OAuthFlowImplementation oAuthFlowImplementation = oAuthImplementationFactory.create(sourceDefinition);
Expand Down