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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 12 commits into from
May 30, 2024
Merged

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

merged 12 commits into from
May 30, 2024

Conversation

aknuds1
Copy link
Contributor

@aknuds1 aknuds1 commented Apr 24, 2024 •
edited
Loading

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 mentioned this pull request Apr 24, 2024
Closed
@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
@aknuds1
Convert to shell script generator
5de9156 
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@aknuds1
Add gopatch to build image
693e168 
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@github-actions GitHub Actions
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.

aknuds1 and others added 2 commits May 29, 2024 11:51
@aknuds1
Update build image version to pr7957-d7652788a7
ff43978
@aknuds1
Add import directive
4b8de56 
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@github-actions GitHub Actions
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
@aknuds1
Fix license
ab8af42 
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@github-actions GitHub Actions
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)?

pstibrany
pstibrany approved these changes May 29, 2024
View reviewed changes
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
@github-actions GitHub Actions
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
@github-actions GitHub Actions
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
Add feature flag -distributor.direct-otlp-translation-enabled
361c5ae 
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@aknuds1 aknuds1 force-pushed the arve/otlp-direct-to-mimir branch from 488e83a to 361c5ae Compare May 30, 2024 11:46
@github-actions GitHub Actions
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.

pstibrany
pstibrany approved these changes May 30, 2024
View reviewed changes
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
@aknuds1 @narqo
OTLP: Use optimized OTel metric translator, converting directly to Mi…
0208296 
…mir format (grafana#7957)

* Use optimized OTLP translator, converting directly to Mimir format

---------

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pstibrany pstibrany pstibrany approved these changes

@grafanabot grafanabot Awaiting requested review from grafanabot grafanabot is a code owner

@wilfriedroset wilfriedroset Awaiting requested review from wilfriedroset wilfriedroset is a code owner

@vaxvms vaxvms Awaiting requested review from vaxvms vaxvms is a code owner

@bubu11e bubu11e Awaiting requested review from bubu11e bubu11e is a code owner

@krajorama krajorama Awaiting requested review from krajorama

@jdbaldry jdbaldry Awaiting requested review from jdbaldry

Assignees
No one assigned
Labels
area/opentelemetry component/distributor type/performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aknuds1 @pstibrany @krajorama