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

Add feature-gate to delete original time field from parsed container logs #33946

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Jul 8, 2024

Description:

This PR adds support for removing the original time attribute/field from the parsed log record as it was suggested at #33389.
Users should use the Timestamp field which holds the parsed time.

This patch introduces this change behind a feature flag called filelog.container.removeOriginalTimeField which is disabled by default following the feature lifecycle.

Link to tracking Issue: #33389

Testing:

  1. Added unit-test
  2. Run with ./bin/otelcontribcol_linux_amd64 --config container_config.yaml --feature-gates=filelog.container.removeOriginalTimeField and verify the output:
2024-07-08T14:26:50.936+0300	info	LogsExporter	{"kind": "exporter", "data_type": "logs", "name": "debug", "resource logs": 1, "log records": 2}
2024-07-08T14:26:50.936+0300	info	ResourceLog #0
Resource SchemaURL: 
Resource attributes:
     -> k8s.namespace.name: Str(some)
     -> k8s.pod.name: Str(kube-controller-kind-control-plane)
     -> k8s.container.restart_count: Str(1)
     -> k8s.pod.uid: Str(49cc7c1fd3702c40b2686ea7486091d6)
     -> k8s.container.name: Str(kube-controller)
ScopeLogs #0
ScopeLogs SchemaURL: 
InstrumentationScope  
LogRecord #0
ObservedTimestamp: 2024-07-08 11:26:50.836517638 +0000 UTC
Timestamp: 2029-03-30 08:31:20.545192187 +0000 UTC
SeverityText: 
SeverityNumber: Unspecified(0)
Body: Str(INFO: log line here)
Attributes:
     -> logtag: Str(F)
     -> log.file.path: Str(/var/log/pods/some_kube-controller-kind-control-plane_49cc7c1fd3702c40b2686ea7486091d6/kube-controller/1.log)
     -> log.iostream: Str(stdout)
Trace ID: 
Span ID: 
Flags: 0

Documentation: Added.

@ChrsMark ChrsMark requested a review from djaglowski as a code owner July 8, 2024 12:04
@ChrsMark ChrsMark requested a review from a team July 8, 2024 12:04
@ChrsMark ChrsMark force-pushed the remove_time_from_container_attributes branch 3 times, most recently from cfe7005 to 7e9d93b Compare July 8, 2024 12:46
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

A feature gate seems a little excessive here because the operator is very new and the information being deleted is available in a more helpful form elsewhere on the log. I'm fine with leaving it if you prefer but also ok with simplifying.

@ChrsMark
Copy link
Member Author

ChrsMark commented Jul 8, 2024

I'm in two minds about the feature gate :). My only concern is about potentially breaking the Helm preset which switched to the container operator as well. @TylerHelmuth's any thoughts on this since this could affect the Helm chart users?

If we agree on this I'm happy to remove the feature gate and land it with the direct deletion.

Copy link
Contributor

@rogercoll rogercoll left a comment

Choose a reason for hiding this comment

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

LGTM! Just a small comment

@@ -78,6 +88,14 @@ func (c Config) Build(set component.TelemetrySettings) (operator.Operator, error
}
}

if removeOriginalTimeField.IsEnabled() {
Copy link
Contributor

@rogercoll rogercoll Jul 9, 2024

Choose a reason for hiding this comment

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

Shouldn't we log this message when not using the feature gate instead? That way would make the user that is not using the gate, aware of the future removal.

Suggested change
if removeOriginalTimeField.IsEnabled() {
if !removeOriginalTimeField.IsEnabled() {

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, that was the intention since the gate will be initially in StageAlpha.
Thank's for catching this!

change_type: 'enhancement'

# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
component: pkg/stanza
Copy link
Member

Choose a reason for hiding this comment

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

It's probably more meaningful for users if we set the component to receiver/filelog? Especially that the feature gate has filelog in its name.

ChrsMark and others added 4 commits July 10, 2024 10:52
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Co-authored-by: Andrzej Stencel <andrzej.stencel@elastic.co>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark force-pushed the remove_time_from_container_attributes branch from 8bfb9ee to 9e73b78 Compare July 10, 2024 07:53
@andrzej-stencel andrzej-stencel merged commit 15c72d2 into open-telemetry:main Jul 10, 2024
154 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 10, 2024
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.

5 participants