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

Update configure-otel-collector.md: update recommendation for OTEL #9769

Conversation

francoposa
Copy link
Member

@francoposa francoposa commented Oct 29, 2024

... to go through otlphttp

What this PR does

Which issue(s) this PR fixes or relates to

Fixes #

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.

@francoposa francoposa requested review from tacole02 and a team as code owners October 29, 2024 17:00
Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

LGTM, modulo nits + typo.

{{% /admonition %}}

When using the [OpenTelemetry Collector](https://opentelemetry.io/docs/collector/), you can write metrics into Mimir via two options: `prometheusremotewrite` and `otlphttp`.

We recommend using the `prometheusremotewrite` exporter when possible because the remote write ingest path is tested and proven at scale.
We recommend using each protocol's respective exporter and native Mimir endpoint:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
We recommend using each protocol's respective exporter and native Mimir endpoint:
As a best practice, use each protocol's respective exporter, as well as the following Mimir endpoints:

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to clear this up a bit. LMK is the meaning is preserved, or if we should try something else.

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 would leave this as is. "We recommend" is the language we use elsewhere in the docs, and I think "Best Practice" implies that a lot more research and consensus than we want to imply here.

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 the endpoints aren't explicitly listed here, those names are for the exporters.
The endpoints are expanded on in each corresponding section below.
We could drop the bit about the endpoints completely, but saying "the following Mimir endpoints" would not make sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying. Let's leave as it is 😄


## Remote Write
- Prometheus data through `prometheusremotewrite`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Prometheus data through `prometheusremotewrite`
- For Prometheus data, `prometheusremotewrite`

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 don't think bullet points need to be worded so close to a full proper sentence, starting it with a prepositional phrase. I do not see other examples of us taking this approach in the docs.

The word "through" could just be a colon if we want to shorten up

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, let's make it a colon, it cleans it up a bit.


## Remote Write
- Prometheus data through `prometheusremotewrite`
- OTel data through `otlphttp`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- OTel data through `otlphttp`
- For OTel data, `otlphttp`

@francoposa francoposa merged commit 0f8b12b into main Oct 30, 2024
29 checks passed
@francoposa francoposa deleted the francoposa/docs-configure-otel-collector-exporter-recommendation branch October 30, 2024 18:23
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.

3 participants