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

Distributor: Move resource attribute promotion specialization to Overrides #10217

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aknuds1
Copy link
Contributor

@aknuds1 aknuds1 commented Dec 11, 2024

What this PR does

Refactoring: Move resource attribute promotion specialization from distributor OTLP handler to validation.Overrides, for better abstraction of the pluggable logic. A couple of tests included. See comment for background.

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 requested a review from colega December 11, 2024 17:52
@aknuds1 aknuds1 requested a review from a team as a code owner December 11, 2024 17:52
@aknuds1 aknuds1 force-pushed the arve/refactor-specialized-promotion branch 2 times, most recently from 48281d9 to 7f8f55e Compare December 11, 2024 19:17
@aknuds1 aknuds1 force-pushed the arve/refactor-specialized-promotion branch from 7f8f55e to e1cb28f Compare December 12, 2024 06:54
@colega
Copy link
Contributor

colega commented Dec 12, 2024

I think this approach is still not great. We're just moving the if condition to a different implementation, instead of using polymorphism and injecting different implementation based on our needs.

All what OTLPHandler needs is this:

type OTelResourceAttributePromotionConfig interface {
	// PromoteOTelResourceAttributes returns which OTel resource attributes to promote for tenant ID.
	PromoteOTelResourceAttributes(id string) []string
}

You already have one implementation for that, which is Overrides. The OOP way of modifying the behavior of the application is not to modify the behavior of Overrides, is to inject a different implementation of OTelResourceAttributePromotionConfig to OTLPHandler on instantiation.

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.

2 participants