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

Use all span kinds as default for metrics #3898

Closed
wants to merge 1 commit into from
Closed

Use all span kinds as default for metrics #3898

wants to merge 1 commit into from

Conversation

ricoberger
Copy link

@ricoberger ricoberger commented Sep 5, 2022

Hi everyone, if this change doesn't make sense or if I missed something to configure the span kind feel free to close the PR.

Which problem is this PR solving?

Background: The span kind for some services could have another value then "SPAN_KIND_SERVER", e.g. the spans from Istio are using "SPAN_KIND_CLIENT". Because of the default span kind and the fact that the span kind is not configureable in the frontend (the query parameter is never set in the API calls) it was not possible to use the service performance monitoring with some services.

Short description of the changes

Instead of just using "SPAN_KIND_SERVER" as the default span kind for the metrics, the default value now includes all span kinds.

Instead of just using "SPAN_KIND_SERVER" as the default span kind for
the metrics, the default value now includes all span kinds.

Background: The span kind for some services could have another value
then "SPAN_KIND_SERVER", e.g. the spans from Istio are using
"SPAN_KIND_CLIENT". Because of the default span kind and the fact that
the span kind is not configureable in the frontend (the query parameter
is never set in the API calls) it was not possible to use the service
performance monitoring with some services.

Signed-off-by: ricoberger <mail@ricoberger.de>
@ricoberger ricoberger requested a review from a team as a code owner September 5, 2022 22:24
@ankitnayan
Copy link

@ricoberger that would create a lot of noise IMHO in the metrics expected in SPM. Say, 1 server call has downstream 1000 calls to other dbs, queues and other services. With the suggested approach, all the spans will be used to calculate metrics for that 1 server call.

@albertteoh
Copy link
Contributor

The jaeger-query API allows callers to override the span kind.

I agree that there are situations where it makes sense to use a span kind other than server; for example, when a job is created to do some work, and we'd like to include spans from that job.

IMO, the more correct place to configure the span kind is in Jaeger UI; either configured on startup or exposed in the UI as a way to override the default server kind filter.

@albertteoh
Copy link
Contributor

Closing this PR for now. Feel free to re-open if you wish to discuss further.

@albertteoh albertteoh closed this Nov 5, 2022
@ricoberger
Copy link
Author

Hi, sorry I completely forgot about this. It's definitely ok to close the PR we tested this further during the last weeks and implemented a solution in our Jaeger frontend, where the user can select the span kind.

@albertteoh
Copy link
Contributor

we tested this further during the last weeks and implemented a solution in our Jaeger frontend, where the user can select the span kind.

Nice one @ricoberger! Would you be open to contributing what you've done to the /~https://github.com/jaegertracing/jaeger-ui/ project? I think it's a valid use case to query on non-server span kinds and would be valuable to others in the community.

@nifrasinnovent
Copy link

@albertteoh a dropdown in Jaeger UI's Monitor tab to select the span kind, and perhaps cache that selection per service
This feature is applicable, if not is there any workaround to accomplish this for now?

albertteoh added a commit to jaegertracing/jaeger-ui that referenced this pull request Dec 4, 2023
## Which problem is this PR solving?
- Resolves jaegertracing/jaeger#4035
- Related to jaegertracing/jaeger#3898

## Description of the changes
- Add a span kind filter to the Monitor tab.
- As span kinds are an attribute of a span, much like the service name,
I thought it made sense to add the dropdown filter next to the service
filter.
- I'm not a designer so open to ideas on how to make this better!

## How was this change tested?
- Manually tested as per the following screen recording.
- Update tests.


/~https://github.com/jaegertracing/jaeger-ui/assets/26584478/2d8ee49f-e30c-4438-9e92-9012814c495d

## Checklist
- [x] I have read
/~https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: albertteoh <see.kwang.teoh@gmail.com>
Signed-off-by: Albert Teoh <albert@packsmith.io>
Co-authored-by: Albert Teoh <albert@packsmith.io>
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.

4 participants