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

OTLP: Use optimized OTel metric translator, converting directly to Mimir format #7957

Merged
merged 12 commits into from
May 30, 2024

Conversation

aknuds1
Copy link
Contributor

@aknuds1 aknuds1 commented Apr 24, 2024

What this PR does

Use optimized OTel metric translator in OTLP endpoint, converting directly to Mimir format.

The translation code is generated by downloading Go sources from mimir-prometheus, and transforming them to use Mimir protobuf types and enums via the gopatch tool. The generated files (_generated.go suffix) are checked into Git, and CI verifies that they are up to date with the mimir-prometheus version depended on.

The workflow should be that when we update the mimir-prometheus dependency, we should also execute make generate-otlp, to make sure we have the latest translation code. It's a minor hassle, but I think the performance gains are worth it.

TODO

  • Modify CI check to generate code and check difference (suggested by @pracucci)
  • Include gopatch in build image
  • Put behind feature gate, to be on the safe side

Benchmark results

✗ benchstat main-otlphandler.txt direct-otlphandler.txt 
goos: linux
goarch: amd64
pkg: github.com/grafana/mimir/pkg/distributor
cpu: Intel(R) Xeon(R) Platinum 8358 CPU @ 2.60GHz
                        │ main-otlphandler.txt │      direct-otlphandler.txt       │
                        │        sec/op        │   sec/op     vs base              │
OTLPHandler/protobuf-32            415.2µ ± 1%   391.5µ ± 0%  -5.71% (p=0.002 n=6)
OTLPHandler/JSON-32                606.9µ ± 1%   583.4µ ± 1%  -3.88% (p=0.002 n=6)
geomean                            502.0µ        477.9µ       -4.80%

                        │ main-otlphandler.txt │       direct-otlphandler.txt        │
                        │         B/op         │     B/op      vs base               │
OTLPHandler/protobuf-32           391.0Ki ± 0%   272.8Ki ± 0%  -30.24% (p=0.002 n=6)
OTLPHandler/JSON-32               473.1Ki ± 0%   354.8Ki ± 0%  -25.00% (p=0.002 n=6)
geomean                           430.1Ki        311.1Ki       -27.67%

                        │ main-otlphandler.txt │      direct-otlphandler.txt       │
                        │      allocs/op       │  allocs/op   vs base              │
OTLPHandler/protobuf-32            5.129k ± 0%   5.122k ± 0%  -0.14% (p=0.002 n=6)
OTLPHandler/JSON-32                8.162k ± 0%   8.155k ± 0%  -0.09% (p=0.002 n=6)
geomean                            6.470k        6.463k       -0.11%

Which issue(s) this PR fixes or relates to

Checklist

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

@aknuds1 aknuds1 force-pushed the arve/otlp-direct-to-mimir branch 9 times, most recently from 144d40d to 6ecf12a Compare April 30, 2024 17:28
@aknuds1 aknuds1 force-pushed the arve/otlp-direct-to-mimir branch 3 times, most recently from 383bb02 to 67cc712 Compare May 1, 2024 11:16
@aknuds1 aknuds1 force-pushed the arve/otlp-direct-to-mimir branch 2 times, most recently from 954bd60 to c4ec41e Compare May 7, 2024 08:20
@aknuds1 aknuds1 force-pushed the arve/otlp-direct-to-mimir branch 6 times, most recently from 88ba95d to 2ea6c1a Compare May 14, 2024 06:36
@aknuds1 aknuds1 changed the title WIP: OTLP: Use optimized OTel metric translator, converting directly to Mimir format OTLP: Use optimized OTel metric translator, converting directly to Mimir format May 14, 2024
@aknuds1 aknuds1 marked this pull request as ready for review May 14, 2024 07:05
Copy link
Contributor

Building new version of mimir-build-image. After image is built and pushed to Docker Hub, a new commit will automatically be added to this PR with new image version grafana/mimir-build-image:pr7957-d7652788a7. This can take up to 1 hour.

Copy link
Contributor

Not building new version of mimir-build-image. This PR modifies mimir-build-image/Dockerfile, but the image grafana/mimir-build-image:pr7957-d7652788a7 already exists.

@aknuds1 aknuds1 requested a review from pstibrany May 29, 2024 13:02
Signed-off-by: Arve Knudsen <[email protected]>
Copy link
Contributor

Not building new version of mimir-build-image. This PR modifies mimir-build-image/Dockerfile, but the image grafana/mimir-build-image:pr7957-d7652788a7 already exists.

@aknuds1
Copy link
Contributor Author

aknuds1 commented May 29, 2024

@pstibrany do you think there's any need to test this in a dev cell first, or should we be good to go (as it's just a transformation from Prom sources)?

Copy link
Member

@pstibrany pstibrany 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 Arve, nice work!

@pstibrany
Copy link
Member

@pstibrany do you think there's any need to test this in a dev cell first, or should we be good to go (as it's just a transformation from Prom sources)?

As per Slack discussion, we may want to have a temporary feature flag to enable new, or use old translation code, in case we find a problem with the new code. After we successfully roll this out to our prod, we can remove the flag and old code.

@aknuds1 aknuds1 requested a review from jdbaldry as a code owner May 30, 2024 11:37
Copy link
Contributor

Not building new version of mimir-build-image. This PR modifies mimir-build-image/Dockerfile, but the image grafana/mimir-build-image:pr7957-d7652788a7 already exists.

@aknuds1 aknuds1 force-pushed the arve/otlp-direct-to-mimir branch from ec1a73f to 488e83a Compare May 30, 2024 11:44
Copy link
Contributor

Not building new version of mimir-build-image. This PR modifies mimir-build-image/Dockerfile, but the image grafana/mimir-build-image:pr7957-d7652788a7 already exists.

@aknuds1 aknuds1 force-pushed the arve/otlp-direct-to-mimir branch from 488e83a to 361c5ae Compare May 30, 2024 11:46
Copy link
Contributor

Not building new version of mimir-build-image. This PR modifies mimir-build-image/Dockerfile, but the image grafana/mimir-build-image:pr7957-d7652788a7 already exists.

@aknuds1
Copy link
Contributor Author

aknuds1 commented May 30, 2024

As per Slack discussion, we may want to have a temporary feature flag to enable new, or use old translation code, in case we find a problem with the new code. After we successfully roll this out to our prod, we can remove the flag and old code.

@pstibrany flag added w/ true for default (is false preferable?), PTAL.

Copy link
Member

@pstibrany pstibrany 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.

@aknuds1 aknuds1 merged commit 8e5f25e into main May 30, 2024
30 checks passed
@aknuds1 aknuds1 deleted the arve/otlp-direct-to-mimir branch May 30, 2024 13:33
narqo pushed a commit to narqo/grafana-mimir that referenced this pull request Jun 6, 2024
…mir format (grafana#7957)

* Use optimized OTLP translator, converting directly to Mimir format

---------

Signed-off-by: Arve Knudsen <[email protected]>
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.

3 participants