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

[pkg/stanza] handle errors from Operator.Process #33847

Merged
merged 7 commits into from
Jul 9, 2024

Conversation

zeitlinger
Copy link
Member

Link to tracking Issue: Fixes #33783

Testing:

nothing added

Documentation:

not applicable

@zeitlinger
Copy link
Member Author

seems some tests expect some errors to be ignored

Test:

assert.NoError(t, reader.Process(context.Background(), []byte("# matches header regex but not metadata operator assumptions\n"), attrs))

Error that now shows:
return p.parser(m, p.delimiter, p.pairDelimiter)

@djaglowski djaglowski added the Run Windows Enable running windows test on a PR label Jul 2, 2024
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.

Generally looks good to me

pkg/stanza/adapter/benchmark_test.go Outdated Show resolved Hide resolved
@jpkrohling jpkrohling changed the title handle errors from Operator.Process [pkg/stanza] handle errors from Operator.Process Jul 4, 2024
@zeitlinger
Copy link
Member Author

seems some tests expect some errors to be ignored

Test:

assert.NoError(t, reader.Process(context.Background(), []byte("# matches header regex but not metadata operator assumptions\n"), attrs))

Error that now shows:

return p.parser(m, p.delimiter, p.pairDelimiter)

here's what I tried - but I'm not sure this is the right path - and it's not working

Subject: [PATCH] add example for using recombine in container operator
---
Index: pkg/stanza/operator/helper/parser.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/pkg/stanza/operator/helper/parser.go b/pkg/stanza/operator/helper/parser.go
--- a/pkg/stanza/operator/helper/parser.go	(revision d5550419676f27a422bb0d2f44e1667aec318a01)
+++ b/pkg/stanza/operator/helper/parser.go	(date 1720096829632)
@@ -91,6 +91,7 @@
 	SeverityParser  *SeverityParser
 	TraceParser     *TraceParser
 	ScopeNameParser *ScopeNameParser
+	IgnoreErrors    bool
 }
 
 // ProcessWith will run ParseWith on the entry, then forward the entry on to the next operators.
@@ -105,7 +106,7 @@
 		return p.HandleEntryError(ctx, entry, err)
 	}
 	if skip {
-		return p.Write(ctx, entry)
+		return p.write(ctx, entry)
 	}
 
 	if err = p.ParseWith(ctx, entry, parse); err != nil {
@@ -118,7 +119,15 @@
 		}
 	}
 
-	return p.Write(ctx, entry)
+	return p.write(ctx, entry)
+}
+
+func (p *ParserOperator) write(ctx context.Context, entry *entry.Entry) error {
+	err := p.Write(ctx, entry)
+	if p.IgnoreErrors {
+		return nil
+	}
+	return err
 }
 
 // ParseWith will process an entry's field with a parser function.
Index: pkg/stanza/fileconsumer/internal/header/config.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/pkg/stanza/fileconsumer/internal/header/config.go b/pkg/stanza/fileconsumer/internal/header/config.go
--- a/pkg/stanza/fileconsumer/internal/header/config.go	(revision d5550419676f27a422bb0d2f44e1667aec318a01)
+++ b/pkg/stanza/fileconsumer/internal/header/config.go	(date 1720096829384)
@@ -8,6 +8,7 @@
 	"bytes"
 	"errors"
 	"fmt"
+	"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/operator/parser/keyvalue"
 	"regexp"
 
 	"go.opentelemetry.io/collector/component"
@@ -62,6 +63,11 @@
 		if op.Type() == "filter" {
 			return nil, fmt.Errorf("operator of type filter is not allowed in `metadata_operators`")
 		}
+
+		if op.Type() == "key_value_parser" {
+			parser, _ := op.(*keyvalue.Parser)
+			parser.IgnoreErrors = true
+		}
 	}
 
 	regex, err := regexp.Compile(matchRegex)

if we can't figure this out, then maybe the approach is wrong

@djaglowski djaglowski force-pushed the handle-process-errors branch from 01dcaf1 to 9e602e8 Compare July 8, 2024 14:27
@djaglowski
Copy link
Member

@zeitlinger, I pushed a minor change to the way the header parser processes errors. I think we just need the changelog now

@zeitlinger zeitlinger force-pushed the handle-process-errors branch from d5bbe60 to cbff854 Compare July 9, 2024 10:46
@djaglowski djaglowski merged commit d31bc2e into open-telemetry:main Jul 9, 2024
170 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 9, 2024
djaglowski pushed a commit that referenced this pull request Jul 31, 2024
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
As discussed at
#34295,
error handling can be problematic in stanza package after the recent
changes at
#33847.
Specifically:
1. When
[`Write`](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/pkg/stanza/operator/helper/writer.go#L50)
is called in a for loop, a returned error should not break the for loop.
It should just be logged.
2. When
[`Process`](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/d78d7bb37d0c84da04ac8a83356eb669ecd75a95/pkg/stanza/operator/operator.go#L40)
is called in a for loop, a returned error should not break the for loop.
It should just be logged.

**Link to tracking Issue:** <Issue number if applicable>
#34295

**Testing:** <Describe what testing was performed and which tests were
added.> Added

**Documentation:** <Describe the documentation added.> ~

---------

Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
djaglowski added a commit that referenced this pull request Oct 15, 2024
This PR contains some cleanup following #34720.

This slightly changes some log messages. It also propagates errors
encountered by sending to the next operator (which was done for other
operators in #33847).
sbylica-splunk pushed a commit to sbylica-splunk/opentelemetry-collector-contrib that referenced this pull request Dec 17, 2024
…metry#35461)

This PR contains some cleanup following open-telemetry#34720.

This slightly changes some log messages. It also propagates errors
encountered by sending to the next operator (which was done for other
operators in open-telemetry#33847).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg/stanza processor/logstransform Logs Transform processor receiver/otlpjsonfile Run Windows Enable running windows test on a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

swallowed exception in Operator.Process
4 participants