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

Add OTLP input API #695

Merged
merged 12 commits into from
Jul 14, 2022
Merged

Add OTLP input API #695

merged 12 commits into from
Jul 14, 2022

Conversation

gouthamve
Copy link
Member

@gouthamve gouthamve commented Jan 5, 2022 •
edited
Loading

What this PR does:

OTLP ingest path.

Checklist

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

@gouthamve gouthamve force-pushed the add-otlp-input-mode branch 2 times, most recently from 8673a85 to 3955742 Compare February 1, 2022 11:52
@gouthamve gouthamve marked this pull request as ready for review February 1, 2022 11:52
@gouthamve gouthamve changed the title WIP: Add OTLP input API Add OTLP input API Feb 1, 2022
@gouthamve gouthamve force-pushed the add-otlp-input-mode branch 2 times, most recently from 9e3a070 to 8cb98ab Compare February 1, 2022 12:46
gouthamve
gouthamve commented Feb 1, 2022
View reviewed changes
pkg/util/push/otel.go Outdated Show resolved Hide resolved
pracucci
pracucci reviewed Feb 21, 2022
View reviewed changes
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Good job! This PR is getting in a bunch of vendor updates. What's actually required? For example, I've seen you've upgraded vendored Prometheus, but we have a replace directive about it in go.mod so no vendored Prometheus code has changed.

Few more things, please:

  • Could you add a CHANGELOG entry?
  • Could me list it among the experimental features listed at docs/sources/configuration/about-versioning.md?

pkg/api/api.go Outdated
@@ -228,6 +228,7 @@ func (a *API) RegisterDistributor(d *distributor.Distributor, pushConfig distrib
distributorpb.RegisterDistributorServer(a.server.GRPC, d)

a.RegisterRoute("/api/v1/push", push.Handler(pushConfig.MaxRecvMsgSize, a.sourceIPs, a.cfg.SkipLabelNameValidationHeader, a.cfg.wrapDistributorPush(d)), true, "POST")
a.RegisterRoute("/api/v1/push/otlp/v1/metrics", push.HandlerForOTLP(pushConfig.MaxRecvMsgSize, a.sourceIPs, a.cfg.SkipLabelNameValidationHeader, a.cfg.wrapDistributorPush(d)), true, "POST")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why two "v1" in the path? Also have you considered keeping "/push" at the end, so something like "/api/v1/otlp/metrics/push"?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is mainly because OTLP expects v1/metrics at the end: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/otlp.md#otlphttp

Can we do another route though?

pkg/api/api.go Outdated Show resolved Hide resolved
pkg/util/push/otel.go Show resolved Hide resolved
pkg/util/push/otel.go Outdated Show resolved Hide resolved
pkg/util/push/otel.go Outdated Show resolved Hide resolved
pkg/util/push/otel.go Show resolved Hide resolved
pkg/util/push/otel.go Outdated Show resolved Hide resolved
pkg/util/push/push.go Outdated Show resolved Hide resolved
@pracucci
Copy link
Collaborator

This is a copy-paste message.

I've just cut the CHANGELOG in preparation to Mimir 2.0.0 release. Could you rebase main and make sure any CHANGELOG entry added by this PR is at the top of the CHANGELOG, under the "main / unreleased" section, please?

@gouthamve gouthamve force-pushed the add-otlp-input-mode branch 5 times, most recently from 01afbc0 to a780e4b Compare February 22, 2022 14:23
@gouthamve
Copy link
Member Author

@pracucci Thanks a lot for the review, I've addressed all your comments and I think this is now good to go!

This PR is getting in a bunch of vendor updates

I've checked those, and I am not sure if I should look at indirect dependencies as well. I've looked at direct dependencies and:

  • github.com/prometheus/client_golang --> v1.11.0 to v1.12.1 seems like a good one
  • google.golang.org/grpc --> v1.41.1 to v1.43.0 I tried reverting this but a go mod tidy puts it back at 1.43.0. I am assuming this is due to the OTel Collector imports

Should I revert the Prometheus update or just keep it as-is?

@gouthamve gouthamve force-pushed the add-otlp-input-mode branch 3 times, most recently from fd9f038 to 5062ac1 Compare February 22, 2022 16:21
replay
replay reviewed Feb 24, 2022
View reviewed changes
pkg/util/push/push_test.go Outdated Show resolved Hide resolved
replay
replay reviewed Feb 24, 2022
View reviewed changes
pkg/util/push/otel.go Outdated Show resolved Hide resolved
@replay
Copy link
Contributor

replay commented Feb 24, 2022

This look great, I only had minor comments

@CLAassistant
Copy link

CLAassistant commented Mar 31, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@pstibrany
Copy link
Member

Please rebase this PR on top of main to make all required test checks pass.

@gouthamve gouthamve force-pushed the add-otlp-input-mode branch 2 times, most recently from 3caff73 to 9232911 Compare March 31, 2022 15:48
@gouthamve
Copy link
Member Author

Hi @pracucci @replay Thanks for the intial reviews, and sorry for taking so long to get back to this. But I think this is ready for a final review!

@gouthamve
Fix botched rebase
6e81969 
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@gouthamve
Dedupe tests code
58d1d21 
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@gouthamve
Use a http specific limit reader
59a92bf 
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@gouthamve gouthamve force-pushed the add-otlp-input-mode branch 2 times, most recently from ed76fb3 to a40aa9d Compare July 12, 2022 12:09
@gouthamve
Update to latest versions
8f2b27b 
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@gouthamve
Add a docker-compose file for development
574e2ba 
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
colega
colega reviewed Jul 13, 2022
View reviewed changes
development/mimir-with-otlp/docker-compose.yml
Comment on lines +31 to +39
otel-collector:
image: otel/opentelemetry-collector-contrib:0.54.0
command: ["--config=/etc/otel-collector/otel-collector.yaml"]
depends_on:
- mimir-1
volumes:
- ./config:/etc/otel-collector
ports:
- 8083:8083
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this otel-collector the only things different in mimir-with-oltp?

I have concerns about having to maintain one more development setup which is probably the same as the one for s3 + otel-collector.

Maybe I'm missing something, then maybe we could have a README.md in this development/mimir-with-oltp that would clarify?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have the same concern. I don't understand the need of a dedicated dev env.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would just remove development/mimir-with-otlp/ and add OT collector to development/tsdb-blocks-storage-s3.

colega
colega reviewed Jul 13, 2022
View reviewed changes
pkg/util/push/otel.go Outdated Show resolved Hide resolved
colega
colega reviewed Jul 13, 2022
View reviewed changes
pkg/util/push/otel.go Outdated Show resolved Hide resolved
@colega colega mentioned this pull request Jul 13, 2022
Closed
@colega
Copy link
Contributor

colega commented Jul 13, 2022 •
edited
Loading

Can you also update the documentation please? Edit: could be a separate review also, to include docs squad in a narrower changeset.

@gouthamve
Copy link
Member Author

I've addressed your changes. I'll add the docs in a separate PR yes.

colega
colega reviewed Jul 13, 2022
View reviewed changes
pkg/util/push/otel.go Outdated Show resolved Hide resolved
colega
colega approved these changes Jul 13, 2022
View reviewed changes
Copy link
Contributor

@colega colega left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@gouthamve
Review feedback
9a4aeab 
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@colega colega merged commit 8e900c2 into main Jul 14, 2022
@colega colega deleted the add-otlp-input-mode branch July 14, 2022 06:39
pracucci
pracucci reviewed Jul 15, 2022
View reviewed changes
pkg/util/push/otel.go
messageSizeLargerErrFmt = "received message larger than max (%d > %d)"
)

func OLTPHandler(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we have a typo here. Shouldn't be OTLPHandler()?

pracucci
pracucci reviewed Jul 15, 2022
View reviewed changes
pkg/api/api.go
wrappedDistributor := a.cfg.wrapDistributorPush(d)

a.RegisterRoute("/api/v1/push", push.Handler(pushConfig.MaxRecvMsgSize, a.sourceIPs, a.cfg.SkipLabelNameValidationHeader, wrappedDistributor), true, false, "POST")
a.RegisterRoute("/api/v1/push/otlp/v1/metrics", push.OLTPHandler(pushConfig.MaxRecvMsgSize, a.sourceIPs, a.cfg.SkipLabelNameValidationHeader, wrappedDistributor), true, false, "POST")
Copy link
Collaborator

Choose a reason for hiding this comment

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

/api/v1/push/otlp/v1/metrics looks weird to me, with that double versioning. What if we simply use /otlp/v1/metrics?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Related: #695 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Also works. I'll open a PR with all your feedback now.

gouthamve added a commit that referenced this pull request Jul 15, 2022
@gouthamve
Final feedback from #695 (OTLP)
fbad312 
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@gouthamve gouthamve mentioned this pull request Jul 15, 2022
Merged
pracucci added a commit that referenced this pull request Jul 15, 2022
@gouthamve @pracucci
Final feedback from #695 (OTLP) (#2436)
1baf904 
* Final feedback from #695 (OTLP)

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>

* Fixed client used in integration tests and CHANGELOG entry

Signed-off-by: Marco Pracucci <marco@pracucci.com>

* Added PR number to CHANGELOG

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Co-authored-by: Marco Pracucci <marco@pracucci.com>
@bboreham bboreham mentioned this pull request Oct 31, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pracucci pracucci pracucci left review comments

@replay replay replay approved these changes

@colega colega colega approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@gouthamve @pracucci @replay @CLAassistant @pstibrany @colega