-
Notifications
You must be signed in to change notification settings - Fork 526
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
otlp: Add beta support to receive logs #6768
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
110e612
to
017039d
Compare
Adds support to receive OTLP logs on the GRPC receiver. This feature is _beta_ and should be used sparsely until the OTel spec declares it as stable. Currently, it's challenging to test this in the system tests since the `opentelemetry-go` SDK doesn't expose any APIs for logs. Signed-off-by: Marc Lopez Rubio <[email protected]>
6808b14
to
835558e
Compare
Signed-off-by: Marc Lopez Rubio <[email protected]>
835558e
to
25aec1f
Compare
if event.Span == nil { | ||
event.Span = &model.Span{} | ||
} | ||
event.Span.ID = spanID.HexString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't this also refer to the transaction.ID
. Is there any information available that would allow a similar check as https://github.com/elastic/apm-server/blob/master/processor/otel/traces.go#L211-L226?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can know that here, but happy to be told otherwise. We're going to need to do something like elastic/apm#413, renaming transaction.id
to span.id
.
I think it might be best to set span.id
even though it might not be correct, i.e. because it's a transaction. At least then users can search for a transaction with the span.id
value, and we'll be aligned for a future where transaction.id
doesn't exist.
@felixbarny WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM. We could also add a span.id
to transactions and look for ways how to phase out transaction.id
going forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good, thanks
if event.Span == nil { | ||
event.Span = &model.Span{} | ||
} | ||
event.Span.ID = spanID.HexString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can know that here, but happy to be told otherwise. We're going to need to do something like elastic/apm#413, renaming transaction.id
to span.id
.
I think it might be best to set span.id
even though it might not be correct, i.e. because it's a transaction. At least then users can search for a transaction with the span.id
value, and we'll be aligned for a future where transaction.id
doesn't exist.
@felixbarny WDYT?
Signed-off-by: Marc Lopez Rubio <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you 🎉
Can you please add the fields to app_logs, unless there's a reason not to?
This pull request is now in conflicts. Could you fix it @marclop? 🙏
|
Signed-off-by: Marc Lopez Rubio <[email protected]>
Adds support to receive OTLP logs on the GRPC receiver. This feature is _beta_ and should be used sparsely until the OTel spec declares it as stable. Signed-off-by: Marc Lopez Rubio <[email protected]> (cherry picked from commit 5e5623e) # Conflicts: # changelogs/head.asciidoc
Adds support to receive OTLP logs on the GRPC receiver. This feature is _beta_ and should be used sparsely until the OTel spec declares it as stable. Signed-off-by: Marc Lopez Rubio <[email protected]> (cherry picked from commit 5e5623e)
This is awesome! Is there a beta build we could try? Thank you! |
@cristianmad We'll publish a release candidate ( |
confirmed with 2d1c270 |
Motivation/summary
Adds support to receive OTLP logs on the GRPC receiver. This feature is
beta and should be used sparsely until the OTel spec declares it as
stable.
Currently, it's challenging to test this in the system tests since the
opentelemetry-go
SDK doesn't expose any APIs for logs.Checklist
apmpackage
have been made)How to test these changes
docker-compose up -d
make
./apm-server -E output.elasticsearch.username=admin -E output.elasticsearch.password=changeme -e
go.opentelemetry.io/collector/[email protected]
):Observability > Logs
UI.Related issues
Closes #5491