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

Add a startTime and endTime parameter to the trace by id query API to improve query performance #1388

Merged
merged 6 commits into from
Apr 18, 2022

Conversation

sagarwala
Copy link
Contributor

@sagarwala sagarwala commented Apr 15, 2022

What this PR does:
This PR adds a startTime and endTime parameter to the Trace By ID API call, to restrict the number of blocks parsed while querying based on the time of the block. Default behaviour remains same if the time window is not specified.
The API spec would now be /api/traces/{traceID}?timeEnd={end}&timeStart={start}

Which issue(s) this PR fixes:
Fixes #1373

cc : @bikashmishra100 @ashwinidulams

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@CLAassistant
Copy link

CLAassistant commented Apr 15, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Overall this is pretty close. I appreciate the work that's done here. We are close to cutting 1.4 and if we hustle we can get this feature in that release (which I would like).

In addition to the requested changes we also need to add these new params to our docs:

https://github.com/grafana/tempo/blob/main/docs/tempo/website/api_docs/_index.md?plain=1#L100

It's also very important that while we describe these new fields carefully in the docs. If the start/end parameters are provided it's quite possible that the trace will not be found or returned partially.

We should also mention the interaction between these new params and this config option:

https://github.com/grafana/tempo/blob/main/docs/tempo/website/configuration/_index.md?plain=1#L776

If a span is received that is outside that slack configuration value then the block start/end times are not updated. This currently has no impact on trace by id search b/c it searches everywhere.

However, after this change, if you push a trace with dates from last week and then search for that trace using /api/traces/<id>?start=<val>&end=<val> then it will not be returned even if start/end correctly overlap with the trace.

Thanks for this feature addition. Let's clean this up some and try to get it merged for 1.4!

modules/querier/http.go Outdated Show resolved Hide resolved
modules/querier/http.go Outdated Show resolved Hide resolved
modules/querier/http.go Outdated Show resolved Hide resolved
modules/querier/http.go Outdated Show resolved Hide resolved
modules/querier/http.go Outdated Show resolved Hide resolved
tempodb/tempodb_test.go Outdated Show resolved Hide resolved
tempodb/tempodb_test.go Outdated Show resolved Hide resolved
tempodb/tempodb_test.go Outdated Show resolved Hide resolved
tempodb/tempodb_test.go Outdated Show resolved Hide resolved
tempodb/tempodb_test.go Outdated Show resolved Hide resolved
@mdisibio mdisibio added this to the v1.4 milestone Apr 15, 2022
@sagarwala
Copy link
Contributor Author

Thank you for the review comments @joe-elliott.

I have updated the documentation to include start/end parameters and added details on ingestion_time_range_slack configuration. The ValidateAndSanitizeRequest method is now added to package/api and I am using the same for frontend as well as querier request validation. The test cases are also updated to include the start and end parameters.

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

a few more nits and we'll be good to merge. thanks!

modules/querier/http.go Outdated Show resolved Hide resolved
tempodb/tempodb_test.go Show resolved Hide resolved
docs/tempo/website/api_docs/_index.md Show resolved Hide resolved
@sagarwala
Copy link
Contributor Author

Updated the documentation and tests as per PR comments. Thanks much @joe-elliott for your time and review.

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Two small changes and we'll merge!

  • Move the warning in the docs. (I added a note on the exact line I'm concerned about).
  • Add a change log entry

docs/tempo/website/api_docs/_index.md Outdated Show resolved Hide resolved
@sagarwala
Copy link
Contributor Author

Done! Thank you!

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Really cool enhancement. Great addition @sagarwala!

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.

Add a startTime and endTime parameter to the trace by id query API to improve query performance
4 participants