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

Tempo: Support multiple filter expressions for service graph queries #81037

Merged
merged 4 commits into from
Jan 26, 2024

Conversation

domasx2
Copy link
Contributor

@domasx2 domasx2 commented Jan 23, 2024

What is this feature?

This PR adds ability pro provide multiple promql filter expressions when making a service map query with Tempo data source. In this case promql OR query is used, result is a union of results for each fitler expression.

target.serviceMapQuery now accepts array of strings in addition to single string. It is fully backwards compat.

Only frontend Tempo data soruce is upgraded, not adding support for multiple filter expressions to service graph query editor at this time.

Why do we need this feature?

App o11y has a need to render service graph that contains all nodes matching given filter criteria + their immediate siblings, and have correct request rates for the edges.

Who is this feature for?

Currently this is needed for app o11y plugin.

Which issue(s) does this PR fix?:

Fixes #

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@grafana-pr-automation grafana-pr-automation bot requested review from a team and removed request for a team January 23, 2024 09:15
@grafana-pr-automation grafana-pr-automation bot requested review from a team and removed request for a team January 23, 2024 09:45
@grafana-pr-automation grafana-pr-automation bot requested review from a team and removed request for a team January 23, 2024 10:05
Copy link
Collaborator

@javiruiz01 javiruiz01 left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@domasx2 domasx2 requested a review from aocenas January 23, 2024 13:36
@@ -67,7 +67,9 @@ export function ServiceGraphSection({
);
}

const filters = queryToFilter(query.serviceMapQuery || '');
const filters = queryToFilter(
(Array.isArray(query.serviceMapQuery) ? query.serviceMapQuery[0] : query.serviceMapQuery) || ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Basically to test this right now I am injecting a second query into buildExpr request.targets[0]?.serviceMapQuery but if you are using App O11y I assume there is some way in the UI to add this or programatically because it returns more precise results.

Only frontend Tempo data soruce is upgraded, not adding support for multiple filter expressions to service graph query editor at this time.

Isn't this confusing if the user is getting back results where they can't see the real query that was run?

App o11y has a need to render service graph that contains all nodes matching given filter criteria + their immediate siblings, and have correct request rates for the edges.

Can you give me an example query/how its used from App O11y where you append/inject (not sure if that's what you do or it's visible in the UI in App O11y) the second part that gets the siblings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

App o11y does not use Tempo service map query editor. We're going for an opinionated experience where queries are created programatically based on context and some high level filters, and are not exposed directly to users.

A specific problem is that customers want to filter app o11y service map by environment (or other attrs), and they want to see nodes that match selected environment + nodes they are directly talking to. So we can't filter this with a single promql filter query, we need to filter serviceMapQuery=['{server_deployment_environment="$env}', '{client_deployment_environment="$env"}'] to get a union of both.

It also enables us to target specifc service and it's sibilings only, by filtering serviceMapQuery=['{server="$name"}', '{client="$name"}']

Related app o11y PR: https://github.com/grafana/app-observability-plugin/pull/767

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, makes sense and thanks for the example 👍

Copy link
Member

@aocenas aocenas left a comment

Choose a reason for hiding this comment

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

I think this makes sense. I guess at some point we should probably have similar UI in the tempo query editor

@domasx2 domasx2 merged commit e9a99a4 into main Jan 26, 2024
20 checks passed
@domasx2 domasx2 deleted the domas-tempo-service-graph-support-or-queries branch January 26, 2024 14:37
Ukochka pushed a commit that referenced this pull request Feb 14, 2024
…81037)

* support "OR" for service graph queries

* make betterer happy

* continue appeasing betterer

* betterer results
@aangelisc aangelisc modified the milestones: 10.4.x, 10.4.0 Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants