[pkg/stanza] Use HandleEntryError for error handling by khushijain21 · Pull Request #35758 · open-telemetry/opentelemetry-collector-contrib

khushijain21 · 2024-10-12T16:36:46Z

Link to tracking issue

Uses HandleEntryError method to handle errors. More reasoning here

ChrsMark

Could we add some basic unit testing for this to ensure that the "mute" logic is preserved in the future?

Also I believe we should have a change-log entry (bug fix probably) for this since it affects what users see.

djaglowski · 2024-10-14T13:32:33Z

The API for parsers is a bit under-constrained but I think it would be cleaner to follow the pattern used in other parsers, where they move the parsing logic into an unexported function and then call that function using ProcessWith. This effectively applies the error handling to any errors returned. Example

ChrsMark · 2024-10-14T13:55:24Z

We already pass the parse method to the ParseWith and ProcessWithCallback :

opentelemetry-collector-contrib/pkg/stanza/operator/parser/container/parser.go

Line 82 in 83aa8bf

err = p.ParserOperator.ProcessWithCallback(ctx, entry, p.parseDocker, p.handleAttributeMappings)
opentelemetry-collector-contrib/pkg/stanza/operator/parser/container/parser.go

Line 118 in 83aa8bf

err = p.ParserOperator.ParseWith(ctx, entry, p.parseContainerd)
opentelemetry-collector-contrib/pkg/stanza/operator/parser/container/parser.go

Line 125 in 83aa8bf

err = p.ParserOperator.ParseWith(ctx, entry, p.parseCRIO)

However the problem here is that we had to parse time after these calls in a custom func to avoid having to redefine the time Parser for each runtime. That's because the time parser is called after the parse function is called:

opentelemetry-collector-contrib/pkg/stanza/operator/helper/parser.go

Line 153 in d31bc2e

timeParseErr = p.TimeParser.Parse(entry)

djaglowski · 2024-10-14T14:15:09Z

Sure, but can't we push all parsing functionality into a single call to ProcessWith?

ChrsMark · 2024-10-14T14:33:46Z

Hmm I think ProcessWith will still end up calling just ParseWith in which the time parsing (with the time.Parser) will happen after the custom parse is called.

However, I think we can do the rest of the custom time parsing etc in a callback function which will be passed to ProcessWithCallback.

djaglowski · 2024-10-14T14:36:51Z

Thanks for taking another look @ChrsMark. I'm a bit rusty on this part of the API so my suggestions are a bit off, but intuitively there must be a more normalized way than applying a special wrapper to every return err.

ChrsMark · 2024-10-15T07:54:38Z

but intuitively there must be a more normalized way than applying a special wrapper to every return err

@djaglowski agreed! I think we can achieve this by moving the extra logic in a callback function passed to ProcessWithCallback. Do you see any possible issue with that?

khushijain21 · 2024-10-16T11:42:01Z

Hello, following the conversation, will this require code refactoring to avoid additional wrapper around each err? And if yes, can you direct how so?

ChrsMark · 2024-10-16T13:04:29Z

Hey @khushijain21 I believe my proposal would worth giving it a try. Unless @djaglowski has a different view.

My proposal would be to move any additional logic that can produce unhandled errors into a callback function and then pass this function to ProcessWithCallback replacing with that the ProcessWith calls.

In this, the ProcessWithCallback will handle any errors produced by the callback function.

Note that ProcessWithCallback is already used at

opentelemetry-collector-contrib/pkg/stanza/operator/parser/container/parser.go

Line 82 in 83aa8bf

    
           err = p.ParserOperator.ProcessWithCallback(ctx, entry, p.parseDocker, p.handleAttributeMappings)

so this will also need to change so as the passed callback to also include the time parsing etc.

Let me know if that makes sense.

djaglowski · 2024-10-16T14:19:09Z

@ChrsMark, thanks, I think what you suggest makes sense.

Co-authored-by: Christos Markou <chrismarkou92@gmail.com>

…llback (#35769)  #### Description Out of the discussions at #35758 I realized that `ProcessWithCallback` does not handle the error returned by the callback function. Since the error of the parse function is handled I think we should handle this of the callback too.  #### Link to tracking issue Fixes  #### Testing  #### Documentation  --------- Signed-off-by: ChrsMark <chrismarkou92@gmail.com>

khushijain21 · 2024-10-30T08:14:33Z

@ChrsMark any update?

ChrsMark · 2024-10-30T09:10:54Z

Unit tests are failing now. I think that's because of the change in HandleEntryError from #35834.

ChrsMark

After #35834 I think that what we have discussed here changes a little bit. I have commented back on the issue as I think we need to revisit the approach.

ChrsMark · 2024-10-30T13:33:16Z

+	}
+
+	if err = p.ParseWith(ctx, entry, parse); err != nil {
+		if p.OnError == DropOnErrorQuiet || p.OnError == SendOnErrorQuiet {


I'm not sure if we should discard the error here. This could lead to duplicating entries as described at #35973.

Otherwise, the container parser will get no error back and will further move the entry to the recombine parser.

ChrsMark · 2024-10-30T13:35:15Z

+			err = p.ParserOperator.ParseWithCallback(ctx, entry, p.parseContainerd, p.cbGoTimeLayout)
 			if err != nil {
-				return fmt.Errorf("failed to parse containerd log: %w", err)
+				return p.HandleEntryError(ctx, entry, stanzaerr.Wrap(err, "containerd log"))


Isn't the error already handled inside the ParseWithCallback? I would assume we should return nil here as the entry has already been either moved forward or dropped.

…ds (#36213)  #### Description This issue was caught at #35758. This PR ensures that time parsing happens before the entry is sent to the next operator in the pipeline.  #### Link to tracking issue Fixes ~  #### Testing Added  #### Documentation ~  Signed-off-by: ChrsMark <chrismarkou92@gmail.com>

github-actions · 2024-11-14T05:20:40Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

…llback (open-telemetry#35769)  #### Description Out of the discussions at open-telemetry#35758 I realized that `ProcessWithCallback` does not handle the error returned by the callback function. Since the error of the parse function is handled I think we should handle this of the callback too.  #### Link to tracking issue Fixes  #### Testing  #### Documentation  --------- Signed-off-by: ChrsMark <chrismarkou92@gmail.com>

…ds (open-telemetry#36213)  #### Description This issue was caught at open-telemetry#35758. This PR ensures that time parsing happens before the entry is sent to the next operator in the pipeline.  #### Link to tracking issue Fixes ~  #### Testing Added  #### Documentation ~  Signed-off-by: ChrsMark <chrismarkou92@gmail.com>

[pkg/stanza] Use HandleEntryError for error handling

49dedbe

khushijain21 requested review from a team and djaglowski as code owners October 12, 2024 16:36

Merge branch 'main' into stanza

b08eca0

github-actions Bot assigned songy23 Oct 12, 2024

github-actions Bot added the pkg/stanza label Oct 12, 2024

ChrsMark reviewed Oct 14, 2024

View reviewed changes

ChrsMark mentioned this pull request Oct 14, 2024

[pkg/stanza] Handle entry error of callback function in ProcessWithCallback #35769

Merged

khushijain21 added 4 commits October 15, 2024 10:51

add test case for send_quiet

88b2d06

cosmetic changes

cd813f7

changelog

5a2055c

add changelog

ae98ac6

khushijain21 requested a review from ChrsMark October 15, 2024 06:56

khushijain21 added 2 commits October 15, 2024 12:40

test reformat

b0c7a38

more test correction

293f94c

Merge branch 'main' into stanza

b3f7327

khushijain21 commented Oct 21, 2024

View reviewed changes

Comment thread pkg/stanza/operator/parser/container/parser.go Outdated

khushijain21 commented Oct 21, 2024

View reviewed changes

Comment thread pkg/stanza/operator/parser/container/parser.go Outdated

khushijain21 added 3 commits October 21, 2024 11:25

add unhandled error

d8a33b3

Merge branch 'main' into stanza

8e72f1b

added parseWithCallback method

833fdfe

Merge branch 'main' into stanza

fa30216

ChrsMark reviewed Oct 22, 2024

View reviewed changes

Comment thread pkg/stanza/operator/parser/container/parser.go

Comment thread pkg/stanza/operator/parser/container/parser.go Outdated

Comment thread pkg/stanza/operator/parser/container/parser.go Outdated

khushijain21 and others added 3 commits October 23, 2024 09:28

Update pkg/stanza/operator/parser/container/parser.go

1abea54

Co-authored-by: Christos Markou <chrismarkou92@gmail.com>

factoring

c4d32ad

Merge branch 'main' into stanza

544df82

khushijain21 requested a review from ChrsMark October 23, 2024 04:30

ChrsMark reviewed Oct 23, 2024

View reviewed changes

Comment thread pkg/stanza/operator/parser/container/parser.go Outdated

Comment thread pkg/stanza/operator/parser/container/parser.go Outdated

khushijain21 and others added 4 commits October 23, 2024 13:29

Update pkg/stanza/operator/parser/container/parser.go

fcdcf19

Co-authored-by: Christos Markou <chrismarkou92@gmail.com>

Update pkg/stanza/operator/parser/container/parser.go

59cd419

Co-authored-by: Christos Markou <chrismarkou92@gmail.com>

refactoring 2

0a07cd1

Merge branch 'main' into stanza

00cec9c

khushijain21 requested a review from ChrsMark October 23, 2024 12:46

Merge branch 'main' into stanza

330c07e

ChrsMark reviewed Oct 30, 2024

View reviewed changes

Comment thread .chloggen/pkg-stanza-error-handling.yaml Outdated

khushijain21 added 3 commits October 30, 2024 18:07

parser.go

28874f5

changelog

f7c965c

Merge branch 'main' into stanza

b1675b0

khushijain21 requested a review from ChrsMark October 30, 2024 13:14

ChrsMark mentioned this pull request Oct 30, 2024

send_quiet is not working for container operator #35726

Closed

ChrsMark reviewed Oct 30, 2024

View reviewed changes

ChrsMark mentioned this pull request Nov 5, 2024

[pkg/stanza] Ensure time parsing happens before entry is sent downwards #36213

Merged

github-actions Bot added the Stale label Nov 14, 2024

khushijain21 closed this Nov 19, 2024

Conversation

khushijain21 commented Oct 12, 2024

Link to tracking issue

Uh oh!

ChrsMark left a comment

Choose a reason for hiding this comment

Uh oh!

djaglowski commented Oct 14, 2024

Uh oh!

ChrsMark commented Oct 14, 2024

Uh oh!

djaglowski commented Oct 14, 2024

Uh oh!

ChrsMark commented Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djaglowski commented Oct 14, 2024

Uh oh!

ChrsMark commented Oct 15, 2024

Uh oh!

khushijain21 commented Oct 16, 2024

Uh oh!

ChrsMark commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djaglowski commented Oct 16, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

khushijain21 commented Oct 30, 2024

Uh oh!

Uh oh!

ChrsMark commented Oct 30, 2024

Uh oh!

ChrsMark left a comment

Choose a reason for hiding this comment

Uh oh!

ChrsMark Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

ChrsMark Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Nov 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ChrsMark commented Oct 14, 2024 •

edited

Loading

ChrsMark commented Oct 16, 2024 •

edited

Loading