-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
Tempo: Fix service graph menu item links #75748
Conversation
…ph-menu-item-links # Conflicts: # public/app/plugins/datasource/tempo/datasource.ts
…ph-menu-item-links # Conflicts: # packages/grafana-data/src/utils/dataLinks.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! But waiting for code owners to approve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would also love to close the menu once an item is clicked but that can be in another PR if you like.
@@ -28,7 +28,11 @@ func (s *Service) getTrace(ctx context.Context, pCtx backend.PluginContext, quer | |||
return nil, err | |||
} | |||
|
|||
request, err := s.createRequest(ctx, dsInfo, model.Query, query.TimeRange.From.Unix(), query.TimeRange.To.Unix()) | |||
if model.Query == nil || *model.Query == "" { | |||
return result, fmt.Errorf("trace id is required") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error message should be updated to reflect that this can be a trace id or a traceql query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only request routed through the backend query()
is the one to retrieve a single trace, so only the trace id makes sense. It's true that in the frontend the query
field can have both a trace id or a TraceQL query but in the current context it can only be a trace id.
if (serviceName !== '') { | ||
query.serviceName = serviceName; | ||
query.filters.push({ | ||
id: 'service-name', | ||
scope: TraceqlSearchScope.Resource, | ||
tag: 'service.name', | ||
value: serviceName, | ||
operator: '=', | ||
valueType: 'string', | ||
}); | ||
} | ||
if (spanName !== '') { | ||
query.spanName = spanName; | ||
query.filters.push({ | ||
id: 'span-name', | ||
scope: TraceqlSearchScope.Span, | ||
tag: 'name', | ||
value: spanName, | ||
operator: '=', | ||
valueType: 'string', | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sets the service/span name in the tags, as opposed to the dedicated Resource Service Name
or Span Name
fields. Users cannot select service/span name in the tags but when it's injected, if they change the tag they cannot reselect either. Also, what happens if they also select service/span name in the dedicated fields.. would the tags override those values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be happening frequently, the id
fields match the default service name and span name filters and it opens correctly locally. Is it possible you have custom "static" fields that are different from the defaults? In that case it would open in the tags section, but that's by design and is expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it's more of an edge case but is possible. I select app in the service graph and then also set the service name myself which results in resource.service.name
being in the query twice. You can't add resource.service.name
in the query twice from the editor. We can merge as is this is not a big issue.
…ph-menu-item-links
What is this feature?
This PR fixes an issue where the service graph menu items wouldn't navigate to the link on click.
It also updates the Tempo link to use the new TraceQL search tab instead of the deprecated search.
Who is this feature for?
Users of the Tempo datasource service graph.
Which issue(s) does this PR fix?:
Fixes #74989
Fixes #74711
Special notes for your reviewer:
Please check that: