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 handler inconsistent behavior with Prometheus OTLP handler #6236

Open
yeya24 opened this issue Sep 25, 2024 · 3 comments
Open

OTLP handler inconsistent behavior with Prometheus OTLP handler #6236

yeya24 opened this issue Sep 25, 2024 · 3 comments

Comments

@yeya24
Copy link
Contributor

yeya24 commented Sep 25, 2024

Describe the bug
Not exactly a bug. It is more a discrepancy between Cortex OTLP handler and Prometheus' OTLP handler. They don't have the same behavior today.

Prometheus OTLP handler has the following configuration enabled https://github.com/prometheus/prometheus/blob/main/storage/remote/write_handler.go#L515

It doesn't convert resource attributes to metric labels automatically but instead use PromoteResourceAttributes to specify which resource attributes to promote to labels. Meanwhile, it enables target_info metric which contains the resource attributes info.

	annots, err := converter.FromMetrics(r.Context(), req.Metrics(), otlptranslator.Settings{
		AddMetricSuffixes:         true,
		PromoteResourceAttributes: otlpCfg.PromoteResourceAttributes,
	})

Cortex is doing the opposite. It disables target_info metric and automatically convert all resource attributes to metric labels https://github.com/cortexproject/cortex/blob/master/pkg/util/push/otlp.go#L41. This could cause cardinality to blow up and cause issues for users who migrate from Prometheus to Cortex.

		setting := prometheusremotewrite.Settings{
			AddMetricSuffixes: true,
			DisableTargetInfo: true,
		}

Proposal

  • Make the default behavior the same as what Prometheus does. Introduce PromoteResourceAttributes Feature Request: OTel resource attribute promotion #6110 and enable target_info metric
  • Add an option to retain the existing behavior to always convert all metric attributes
  • Enable target_info metric or not can be a separate feature flag.
@CharlieTLe
Copy link
Member

I think we should try to be as consistent with Prometheus as possible. I like the proposals.

@SungJin1212
Copy link
Contributor

@yeya24
Can I take it?

@yeya24
Copy link
Contributor Author

yeya24 commented Sep 27, 2024

Sure!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants