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

Jaeger: Search traces #32805

Merged
merged 26 commits into from May 11, 2021
Merged

Jaeger: Search traces #32805

merged 26 commits into from May 11, 2021

Conversation

zoltanbedi
Copy link
Member

@zoltanbedi zoltanbedi commented Apr 8, 2021

What this PR does / why we need it:

This PR implements search query for Jaeger.

TODO

  • Add images to docs
  • Update UI once Nico is ready with his feedback
  • Add tests

Which issue(s) this PR fixes:

Fixes #24953
Fixes #27245

@CLAassistant
Copy link

CLAassistant commented Apr 20, 2021

CLA assistant check
All committers have signed the CLA.

@zoltanbedi zoltanbedi changed the title WIP: Search jaeger traces Jaeger: Search traces Apr 26, 2021
@zoltanbedi zoltanbedi self-assigned this Apr 26, 2021
@zoltanbedi zoltanbedi added this to the 8.0.0 milestone Apr 26, 2021
@zoltanbedi zoltanbedi added this to Under review in Observability (deprecated, use Observability Squad) via automation Apr 26, 2021
@zoltanbedi zoltanbedi marked this pull request as ready for review April 26, 2021 14:08
@zoltanbedi zoltanbedi requested a review from a team April 26, 2021 14:08
@zoltanbedi zoltanbedi requested a review from a team as a code owner April 26, 2021 14:08
@zoltanbedi zoltanbedi requested review from oscarkilhed, peterholmberg, Elfo404 and aocenas and removed request for a team, oscarkilhed and peterholmberg April 26, 2021 14:08
Copy link
Contributor

@achatterjee-grafana achatterjee-grafana left a comment

Choose a reason for hiding this comment

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

Added some review comments.

docs/sources/datasources/jaeger.md Outdated Show resolved Hide resolved
docs/sources/datasources/jaeger.md Outdated Show resolved Hide resolved
docs/sources/datasources/jaeger.md Outdated Show resolved Hide resolved
docs/sources/datasources/jaeger.md Outdated Show resolved Hide resolved
docs/sources/datasources/jaeger.md Outdated Show resolved Hide resolved
Copy link
Contributor

@achatterjee-grafana achatterjee-grafana left a comment

Choose a reason for hiding this comment

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

Another suggestion.

docs/sources/datasources/jaeger.md Outdated Show resolved Hide resolved
Copy link
Contributor

@achatterjee-grafana achatterjee-grafana left a comment

Choose a reason for hiding this comment

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

Added copy edit changes.

docs/sources/datasources/jaeger.md Outdated Show resolved Hide resolved
docs/sources/datasources/jaeger.md Outdated Show resolved Hide resolved
zoltanbedi and others added 4 commits April 30, 2021 11:22
Co-authored-by: achatterjee-grafana <70489351+achatterjee-grafana@users.noreply.github.com>
Copy link
Contributor

@achatterjee-grafana achatterjee-grafana left a comment

Choose a reason for hiding this comment

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

LGTM

@aocenas
Copy link
Member

aocenas commented May 4, 2021

Code looks good but there are some behavior that I think we should try to fix:

  1. The limit placeholder is not seen in full so it's not clear what it means, we should probably put longer description in some info tooltip.
    Screenshot from 2021-05-04 15-21-29

  2. When querying logs the order is not stable and I get random traces every time. This isn't happening in jaeger ui so I assume there is either some param in the API or they sort the traces by time after they get them.

  3. There seems to be some weird behavior with min/max limits but some of that is also present in Jaeger. In Jaeger if I use both min + max it seems like max is ignored. In this PR it seems like max limit is always ignored.

@zoltanbedi
Copy link
Member Author

Code looks good but there are some behavior that I think we should try to fix:

  1. The limit placeholder is not seen in full so it's not clear what it means, we should probably put longer description in some info tooltip.
  2. When querying logs the order is not stable and I get random traces every time. This isn't happening in jaeger ui so I assume there is either some param in the API or they sort the traces by time after they get them.
  3. There seems to be some weird behavior with min/max limits but some of that is also present in Jaeger. In Jaeger if I use both min + max it seems like max is ignored. In this PR it seems like max limit is always ignored.
  1. Added the placeholder text to the info tooltip instead.
  2. It seems like that they sort by most recent traces so I changed it to do the same. Don't we have a field config related to sorting?
  3. I couldn't reproduce this. I see the same behaviour as in the Jaeger UI.

@aocenas
Copy link
Member

aocenas commented May 5, 2021

I couldn't reproduce this. I see the same behaviour as in the Jaeger UI.

hmm yes you are right, but still this seems super weird, I mean this seems like a bug in the Jaeger API but I think this will create some issues for us as people will assume it's a bug in Grafana. @hemdrup @davkal what do you think about this? From perspective of Grafana user this seems broken. Should we allow only one of mix/max at the time? Or some disclaimer?

Another small nitpick:
Would probably change the placeholder to something like foo, bar, baz (or some more real tags) to give an example of the syntax (delimiter) as that is more helpful there imho.
Screenshot from 2021-05-05 15-06-40

@thunderpigeon
Copy link
Contributor

Should we allow only one of mix/max at the time? Or some disclaimer?

  • @aocenas Great idea on the "tag" example
  • @aocenas Not sure what you mean by "only one min/mac" Do you mean multiple allowing filters like? Show me logs between 2-3s + logs between 4-5s?

@aocenas
Copy link
Member

aocenas commented May 6, 2021

@hemdrup I meant that right now if you filter 1ms - 2ms as min max duration you still get traces > 2ms. This bug is also seen in jaeger UI so probably not something we can fix on our side easily, but the thing is this will look like it's broken in Grafana. The limits work though if used separately so I was thinking we could just put a switch there so you can set only one at a time.

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.

We can check the min/max issue in later PR

@zoltanbedi zoltanbedi merged commit 1a504ce into main May 11, 2021
Observability (deprecated, use Observability Squad) automation moved this from Under review to Done May 11, 2021
@zoltanbedi zoltanbedi deleted the zoltan/better-jaeger-search branch May 11, 2021 08:38
ryantxu pushed a commit that referenced this pull request May 13, 2021
* WIP: Search jaeger traces

* Add more customizable query params

* Fix failing test

* Fix e2e test

* Add tags input field

* Minor changes

* Fix tests

* Add docs

* Make sure linking is working

* Add tests to datasource.ts

* Apply suggestions from code review

Co-authored-by: achatterjee-grafana <70489351+achatterjee-grafana@users.noreply.github.com>

* Update Jaeger doc

* UI updates

* Address review comments

* Add new screenshots to docs

* Update placeholder text for tags

Co-authored-by: achatterjee-grafana <70489351+achatterjee-grafana@users.noreply.github.com>
mortenaa pushed a commit to mortenaa/grafana that referenced this pull request May 25, 2021
* WIP: Search jaeger traces

* Add more customizable query params

* Fix failing test

* Fix e2e test

* Add tags input field

* Minor changes

* Fix tests

* Add docs

* Make sure linking is working

* Add tests to datasource.ts

* Apply suggestions from code review

Co-authored-by: achatterjee-grafana <70489351+achatterjee-grafana@users.noreply.github.com>

* Update Jaeger doc

* UI updates

* Address review comments

* Add new screenshots to docs

* Update placeholder text for tags

Co-authored-by: achatterjee-grafana <70489351+achatterjee-grafana@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Support jaeger service and operation search to Grafana querystring Searching jaeger traces by tags
6 participants