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

Frontend Refactor: Streaming tags #3460

Merged
merged 56 commits into from
Mar 20, 2024
Merged

Conversation

joe-elliott
Copy link
Member

What this PR does:
Moves all tags endpoints over to the new frontend async pipeline. Additionally adds:

  • Streaming support to all tags endpoints
  • Cache support for tags
    • See notes. I'm wondering if caching tag values is worth it.
  • Integration tests for streaming tags endpoints
  • CLI support for tags endpoints

Which issue(s) this PR fixes:
Work toward #3400

Checklist

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

Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott joe-elliott marked this pull request as ready for review March 5, 2024 17:36
CHANGELOG.md Outdated Show resolved Hide resolved
docs/sources/tempo/operations/tempo_cli.md Outdated Show resolved Hide resolved
docs/sources/tempo/operations/tempo_cli.md Outdated Show resolved Hide resolved
docs/sources/tempo/operations/tempo_cli.md Outdated Show resolved Hide resolved
cmd/tempo-cli/cmd-query-search-tag-values.go Show resolved Hide resolved
@@ -1,4 +1,5 @@
target: all
stream_over_http_enabled: true
Copy link
Member

Choose a reason for hiding this comment

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

Streaming in the tests is done via gPRC no? Why is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines +112 to +115
go func() {
time.Sleep(50 * time.Millisecond)
cancel()
}()
Copy link
Member

Choose a reason for hiding this comment

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

Why not cancel synchronously? This is adding instability to the tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

these tests are comprehensive, but poorly organized. as i move more endpoints to the new streaming pipeline i want to clean them up but they are becoming quite unruly.

in this case:

  1. we put the http request in the http pipeline here: https://github.com/grafana/tempo/pull/3460/files#diff-512f96e1e3fb3f728660709269d6e2089a469dd022967684aaea6d035a2c4203R117
  2. the go func you've highlighted cancels the context 50ms later
  3. 50ms beats the timeout here: https://github.com/grafana/tempo/blob/main/modules/frontend/search_handlers_test.go#L66
  4. so we expect a context canceled error (which we get)

we need the async go func so we cancel the context while the http request is in flight.

modules/frontend/cache_keys.go Outdated Show resolved Hide resolved
joe-elliott and others added 2 commits March 7, 2024 16:12
Co-authored-by: Mario <mariorvinas@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

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

LGTM

@joe-elliott joe-elliott mentioned this pull request Mar 15, 2024
9 tasks
Comment on lines +128 to +137
Arguments:
- `host-port` A host/port combination for Tempo. The scheme will be inferred based on the options provided.
- `tag` The fully qualified traceql tag to search for. e.g. `resource.service.name`
- `start` Start of the time range to search: (YYYY-MM-DDThh:mm:ss)
- `end` End of the time range to search: (YYYY-MM-DDThh:mm:ss)

Options:
- `--org-id <value>` Organization ID (for use in multi-tenant setup).
- `--use-grpc` Use GRPC streaming
- `--path-prefix <value>` String to prefix search paths with
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the kind of thing you might consider using a definition list, which is supported by Hugo. That the top item (the term) is bold, and the definition appears underneath.

Suggested change
Arguments:
- `host-port` A host/port combination for Tempo. The scheme will be inferred based on the options provided.
- `tag` The fully qualified traceql tag to search for. e.g. `resource.service.name`
- `start` Start of the time range to search: (YYYY-MM-DDThh:mm:ss)
- `end` End of the time range to search: (YYYY-MM-DDThh:mm:ss)
Options:
- `--org-id <value>` Organization ID (for use in multi-tenant setup).
- `--use-grpc` Use GRPC streaming
- `--path-prefix <value>` String to prefix search paths with
Arguments:
`host-port`
: A host/port combination for Tempo. The scheme will be inferred based on the options provided.
`tag`
: The fully qualified traceql tag to search for; for example: `resource.service.name`
`start`
: Start of the time range to search: (`YYYY-MM-DDThh:mm:ss`)
`end`
: End of the time range to search: (`YYYY-MM-DDThh:mm:ss`)
Options:
`--org-id <value>`
: Organization ID (for use in multi-tenant setup).
`--use-grpc`
: Use GRPC streaming
`--path-prefix <value>`
: String to prefix search paths with

Copy link
Member Author

Choose a reason for hiding this comment

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

I like this idea but there are a ton of commands in this file and if we go to definition lists then we should do all at once.

Copy link
Contributor

@knylander-grafana knylander-grafana left a comment

Choose a reason for hiding this comment

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

Thank you for updating the docs! I had a minor formatting suggestion.

joe-elliott and others added 19 commits March 19, 2024 16:04
Co-authored-by: Kim Nylander <104772500+knylander-grafana@users.noreply.github.com>
* cancel context

Signed-off-by: Joe Elliott <number101010@gmail.com>

* update dskit

Signed-off-by: Joe Elliott <number101010@gmail.com>

* focused timeouts

Signed-off-by: Joe Elliott <number101010@gmail.com>

* docs

Signed-off-by: Joe Elliott <number101010@gmail.com>

* lint N docs

Signed-off-by: Joe Elliott <number101010@gmail.com>

* more lint

Signed-off-by: Joe Elliott <number101010@gmail.com>

* make update-mod

Signed-off-by: Joe Elliott <number101010@gmail.com>

---------

Signed-off-by: Joe Elliott <number101010@gmail.com>
Bumps [anchore/sbom-action](https://github.com/anchore/sbom-action) from 0.15.8 to 0.15.9.
- [Release notes](https://github.com/anchore/sbom-action/releases)
- [Commits](anchore/sbom-action@v0.15.8...v0.15.9)

---
updated-dependencies:
- dependency-name: anchore/sbom-action
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* doc: remove reference to previously purged script

* doc: correct label for docs updates
…ation check (grafana#3484)

* Use new per-tenant max_metrics_duration, and fix duration timestamp handling

* Update docs and defaults
* Handle prefixes when listing blocks from S3

fixes grafana#3465

* Handle prefixes when listing blocks from GCS

* Add test for prefixes when listing blocks from Azure

* Update unit tests to check for actual block IDs instead of just length of the slices

Cleanup unit tests

* Further refine S3/GCS backend for ListBlocks

Brings logic more in line with Azure object parsing.
Also has the benefit of handling prefixes without a trailing slash.

* Update poller integration test to exercise prefixes

* Update e2e test to exercise prefixes

* Fix format check error

* Fix failing e2e tests

* Remove unnecessary prefix permutations from e2e test

* Remove unnecessary test config file copy

* Ignore lint

---------

Co-authored-by: Zach Leslie <zach.leslie@grafana.com>
Updates the doc-validator to the latest version. Note that this changes the reference format to use the full URL (https://....) instead of /docs/blah
* [DOC] Document Tempo Operator Monolithic mode

Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>

* clarify supported storages

Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>

* fix case of title

Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>

* Apply suggestions from code review

* Apply suggestions from code review

---------

Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
Co-authored-by: Kim Nylander <104772500+knylander-grafana@users.noreply.github.com>
…ators (grafana#3473)

* [docs] document Grafana data source setup using Grafana and Tempo operators

* move the Grafana data source setup page to the operator folder (this
  page is only relevant for the operator)
* document Grafana data source setup using Grafana and Tempo operators

Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>

* Apply suggestions from code review

---------

Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
Co-authored-by: Kim Nylander <104772500+knylander-grafana@users.noreply.github.com>
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
…na#3455)

* Bump github.com/prometheus/client_golang from 1.18.0 to 1.19.0

Bumps [github.com/prometheus/client_golang](https://github.com/prometheus/client_golang) from 1.18.0 to 1.19.0.
- [Release notes](https://github.com/prometheus/client_golang/releases)
- [Changelog](https://github.com/prometheus/client_golang/blob/main/CHANGELOG.md)
- [Commits](prometheus/client_golang@v1.18.0...v1.19.0)

---
updated-dependencies:
- dependency-name: github.com/prometheus/client_golang
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* Update serverless gomod

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: grafanabot <bot@grafana.com>
…#3458)

* Add support for dashes, quotes and spaces in attribute names

* chlog
* Step align query_range time range

* Time range error: improve message and fix format for prom format.

* oops remove printlns

* lint

* changelog
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
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.

7 participants