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

[pkg/stanza] Improve error logs produced by transformer processors #37285

Merged
merged 16 commits into from
Feb 26, 2025
Merged

[pkg/stanza] Improve error logs produced by transformer processors #37285

merged 16 commits into from
Feb 26, 2025

Conversation

douglascamata
Copy link
Contributor

@douglascamata douglascamata commented Jan 17, 2025 •
edited
Loading

Description

This improves the error messages of the log transformers processors to include the log file path and the original log record, effectively allowing users to quickly debug and investigate deeper the reason for the problem without having to use the debug exporter or using the highest verbosity level.

Here's an example of one of these failures:

2025-01-17T15:38:13.637Z error helper/transformer.go:114 Failed to process entry {"kind": "receiver", "name": "filelog", "data_type": "logs", "operator_id": "move5", "operator_type": "move", "log.file.path": "/var/log/pods/kube-system_kindnet-jxpz6_0784c9f9-ec2b-4829-aeb3-263ec66ef953/kindnet-cni/19.log", "entry.timestamp": "2025-01-17T15:38:07.645938111Z", "error": "move: field does not exist: attributes.uid", "action": "send"}

This log line could be even more helpful if the it included the regex pattern that was matched against the entry. I'm not sure about adding it now and it could be added in the future if desired.

A small note on the name of the log keys: they could have shorter names, like body and file_path but I think we could also use the attribute names from the semantic conventions for logs. I have no preference though and I'm happy to change it if needed.

Testing

  • Ran it locally in a kind cluster.
  • Some unit tests were updated to ensure the log.record.original key is present in the logs.

@github-actions github-actions bot requested a review from djaglowski January 17, 2025 14:27
@douglascamata douglascamata changed the title [pkg/stanza] Add original log record to logs produced by transformer processors [pkg/stanza] Improve error logs produced by transformer processors Jan 17, 2025
@douglascamata douglascamata marked this pull request as ready for review January 17, 2025 15:54
@douglascamata douglascamata requested a review from a team as a code owner January 17, 2025 15:54
@douglascamata
Merge branch 'main' of github.com:open-telemetry/opentelemetry-collec…
578aafb 
…tor-contrib into improve-log-transformers-logs
@douglascamata
Copy link
Contributor Author

I have no idea about why this codeowners check is failing. I didn't touch the CODEOWNERS file and rebased with main a couple times.

djaglowski
djaglowski reviewed Jan 21, 2025
View reviewed changes
pkg/stanza/operator/helper/transformer.go Outdated
zap.Any("action", t.OnError),
}
if entry != nil {
toAddFields := []zap.Field{zap.Any(attrs.LogRecordOriginal, entry.Body)}
Copy link
Member

Choose a reason for hiding this comment

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

I don't agree we should add the body. The body could contain sensitive information and is not necessarily the reason for the failure. Maybe the timestamp is enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to make the Collector's log useful enough so that we could avoid looking at the real log file.

I had totally overlooked the fact that the body can contain sensitive information. So I agree that we can't simply include it there. :/

Originally I wanted to include the line number of the entry, but because of the way the file scanning is working it got complicated (lots of wrapping of the scanner's splitfunc and some file position tracking is byte based instead of line based).

I think the timestamp will be a good substitute for now and I can try to get the line number working later. 👍

@douglascamata
Copy link
Contributor Author

@djaglowski do you have some time to have another look at this, please?

@douglascamata douglascamata mentioned this pull request Jan 31, 2025
Closed
6 tasks
djaglowski
djaglowski reviewed Feb 10, 2025
View reviewed changes
@douglascamata
Copy link
Contributor Author

Hey @djaglowski, sorry for the delay. I took take of all your comments from last week and got the build green again. This is ready for another look.

@djaglowski djaglowski merged commit 6c94334 into open-telemetry:main Feb 26, 2025
162 checks passed
@github-actions github-actions bot added this to the next release milestone Feb 26, 2025
yiquanzhou added a commit to dash0hq/opentelemetry-collector-contrib that referenced this pull request Feb 27, 2025
@yiquanzhou
Merge branch 'main' into add-cloud-provider-attribute
2778429 
* main: (22 commits)
  [receiver/awsfirehose] Add support for encoding extensions (open-telemetry#37262)
  fix(deps): update module google.golang.org/api to v0.223.0 (open-telemetry#38181)
  [chore] skip TestSyslogComplementaryRFC3164 (open-telemetry#38240)
  fix(deps): update module github.com/tencentcloud/tencentcloud-sdk-go/tencentcloud/common to v1.0.1106 (open-telemetry#38199)
  [provider/s3] Use mdatagen, promote to alpha (open-telemetry#38227)
  fix: fix flaky test in kafkatopicsobserver (open-telemetry#38218)
  [processor/resourcedetection] Add k8s.cluster.uid to kubeadm detector (open-telemetry#38216)
  Revert "Add issue generation from fkaly tests for all archs (open-telemetry#38191)" (open-telemetry#38230)
  Revert "Introduce issuegenerator to open issues when tests fail on main (open-telemetry#38177)" (open-telemetry#38231)
  [chore] Update otelcol core dependency (open-telemetry#38214)
  [pkg/stanza] Improve error logs produced by transformer processors (open-telemetry#37285)
  [receiver/statsd] Make full config structure public (open-telemetry#38186)
  processor/metricsstarttime: add ridwanmsharif as codeowner (open-telemetry#38193)
  fix(deps): update module github.com/huaweicloud/huaweicloud-sdk-go-v3 to v0.1.137 (open-telemetry#38154)
  [pkg/datadog] export StaticAPIKeyCheck (open-telemetry#38223)
  [chore][pkg/ottl] Move scope and resource PathGetSetters to internal ctx packages (open-telemetry#38225)
  fix(deps): update all github.com/datadog packages to v0.64.0-rc.3 (open-telemetry#38202)
  feat(telemetrygen): added support for delta temporality (open-telemetry#38146)
  [chore] Some more fixes of component IDs (open-telemetry#38221)
  [chore][pkg/ottl] Define PathGetSetter in ctxdatapoint (open-telemetry#38201)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djaglowski djaglowski djaglowski approved these changes

Assignees

@andrzej-stencel andrzej-stencel

Labels
pkg/stanza
Projects
None yet
Milestone
next release
Development

Successfully merging this pull request may close these issues.
3 participants
@douglascamata @djaglowski @andrzej-stencel