-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
fix: enable service detection for otlp endoint #14036
Conversation
pkg/loghttp/push/otlp.go
Outdated
@@ -135,6 +146,10 @@ func otlpToLokiPushRequest(ctx context.Context, ld plog.Logs, userID string, ten | |||
return true | |||
}) | |||
|
|||
if !hasServiceName && len(discoverServiceName) > 0 && !stats.IsAggregatedMetric { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created the the variable and reused where I could, not sure if the line #s in your links were off, but it I don't think this can replace the hasServiceName
logic, I think we need both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. left a couple of nits
Co-authored-by: Sven Grossmann <Svennergr@gmail.com>
Co-authored-by: Ashwanth <iamashwanth@gmail.com>
Co-authored-by: Ashwanth <iamashwanth@gmail.com>
Co-authored-by: Ashwanth <iamashwanth@gmail.com>
Co-authored-by: Ashwanth <iamashwanth@gmail.com>
Co-authored-by: Ashwanth <iamashwanth@gmail.com>
Co-authored-by: JordanRushing <rushing.jordan@gmail.com>
This PR must be merged before a backport PR will be created. |
Co-authored-by: Sven Grossmann <Svennergr@gmail.com> Co-authored-by: Ashwanth <iamashwanth@gmail.com> Co-authored-by: JordanRushing <rushing.jordan@gmail.com> (cherry picked from commit 4f962ef)
What this PR does / why we need it:
#13702 moved service detection further up in the distributor logic, from here to the push request parsing here. The problem with that is we pass multiple push request parsers to that
pushHandler
, so now service detection was limited to only in one of them, the traditional push path. This PR adds service detection to the OTLP parser, so it happens at the same time in the distributors logic (honoring the spirit of #13702), but now doing so for OTLP pushes as well.Which issue(s) this PR fixes:
Fixes #14035
Special notes for your reviewer:
Checklist
CONTRIBUTING.md
guide (required)feat
PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.md
production/helm/loki/Chart.yaml
and updateproduction/helm/loki/CHANGELOG.md
andproduction/helm/loki/README.md
. Example PRdeprecated-config.yaml
anddeleted-config.yaml
files respectively in thetools/deprecated-config-checker
directory. Example PR