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

Fix OTEL Exception Message Propagation #341

Merged
merged 5 commits into from
Aug 29, 2024

Conversation

rubvs
Copy link
Contributor

@rubvs rubvs commented Aug 9, 2024

Fix on Issue 13603

The issue is reproduced by:

  • Running apm-server locally.
  • Sending OTLP data via the Go client sendotlp.
    • Some changes was made to apm-server/systemtest/cmd/sendotlp
    • I'll open a PR with this in apm-server once this PR is approved
  • View the document in Table form in Kibana -> Analytics -> Discover
incorrect

The issue is fixed by:

  • Applying the changes from apm-data to apm-server
    • cd apm-server
    • go mod edit -replace=github.com/elastic/apm-data=../apm-data
    • make update
  • Redo steps as depicted above.
correct

@obltmachine obltmachine added the safe-to-test Changes are safe to run in the CI label Aug 9, 2024
@rubvs rubvs requested review from simitt and carsonip August 9, 2024 14:39
Copy link
Member

@carsonip carsonip left a comment

Choose a reason for hiding this comment

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

https://github.com/elastic/apm-data/pull/341/files#diff-a83af8750cbc259cb07425b435f9f248022e844152a698eb5aca74dcad7b10a0R26

// SetErrorMessage is a modelpb.BatchProcessor that sets the APMEvent.Message
// field for error events.

Have you figured out why it was implemented like that in the first place? If it is a bug, do you mind updating the comment as well?

@rubvs rubvs requested a review from kruskall August 9, 2024 18:12
@rubvs
Copy link
Contributor Author

rubvs commented Aug 9, 2024

https://github.com/elastic/apm-data/pull/341/files#diff-a83af8750cbc259cb07425b435f9f248022e844152a698eb5aca74dcad7b10a0R26

// SetErrorMessage is a modelpb.BatchProcessor that sets the APMEvent.Message
// field for error events.

Have you figured out why it was implemented like that in the first place? If it is a bug, do you mind updating the comment as well?

@carsonip thanks for pushing me to investigate. I assumed it was a know bug. My thinking was that the error message should be in error.message, hence the extra green highlight.

@kruskall if I'm not mistaken, you added this comment, can you maybe elaborate on this please?

@kruskall
Copy link
Member

Hey @rubvs 👋

The comment was added in this PR (along with the file): 9b6fe76

Historically those processors lived in https://github.com/elastic/apm-server/. Specifically the SetErrorMessage processor was at https://github.com/elastic/apm-server/blob/cdae75cd13183b4c049a0fd1901e4d6a7b4afd67/model/modelprocessor/errormessage.go

It was added by this PR: elastic/apm-server#5974

The original issue mentioned in the PR is at elastic/apm-server#5971.

Based on the issue description I guess setting message is intended.


Looking at the codebase, the event message should be correctly set at

event.Message = body.AsString()

Could you try to comment out the setErrorMessage method call to check if that is working as intended ? Maybe the solution here is to only run the processor if message is empty since it seems the intent of the original issue was to populate the event.message field.

cc @axw since you opened the original issue/PR.

@axw
Copy link
Member

axw commented Aug 14, 2024

Thanks for the ping @kruskall

I think SetErrorMessage predates our support for OTLP logs, and was implemented only with exception span events in mind. In that case message will not be set at all, prior to SetErrorMessage copying the value across.

The point of setting the message field was to ensure the exceptions show up in the Logs app. Not sure if this is necessary any more, but it was back then.

Perhaps the simplest thing to do would be to change SetErrorMessage to only set message if it's empty.

@rubvs rubvs changed the title fix otel exception message propagation (#13603) Fix OTEL Exception Message Propagation Aug 14, 2024
@rubvs rubvs force-pushed the fix-ecs-message-field branch 2 times, most recently from 0522131 to 3c951b0 Compare August 19, 2024 01:37
@rubvs rubvs marked this pull request as ready for review August 19, 2024 01:38
@rubvs rubvs requested a review from a team as a code owner August 19, 2024 01:38
model/modelprocessor/errormessage.go Outdated Show resolved Hide resolved
model/modelprocessor/errormessage_test.go Outdated Show resolved Hide resolved
1pkg
1pkg previously approved these changes Aug 27, 2024
axw
axw previously approved these changes Aug 28, 2024
Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

Thanks! I think we should probably remove the expectedLogMessage and expectedExceptionMessage bits, but otherwise LGTM.

model/modelprocessor/errormessage_test.go Outdated Show resolved Hide resolved
@rubvs rubvs dismissed stale reviews from axw and 1pkg via 1b5ee67 August 29, 2024 13:57
@rubvs rubvs merged commit fcd5ede into elastic:main Aug 29, 2024
5 checks passed
@rubvs rubvs deleted the fix-ecs-message-field branch August 29, 2024 17:27
@inge4pres inge4pres self-assigned this Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe-to-test Changes are safe to run in the CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants