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

feat(observability): OpenTelemetry logs #13291

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

samugi
Copy link
Contributor

@samugi samugi commented Jun 24, 2024

Note for reviewers: commits are split and commented appropriately, I would recommend checking them one by one.

Summary

This PR adds OpenTelemetry formatted logs as a new
signal supported by the OpenTelemetry plugin.

It patches the ngx.log function (and adapts kong PDK log) to collect the logged lines and
enrich them with additional context info:

  • severity
  • introspection details
  • timestamp

And for request-scoped logs also:

  • Request ID
  • Trace ID
  • Span ID
  • Trace flags

The log entries are protobuf-serialized and sent to the endpoint
configured in the OpenTelemetry plugin's config.logs_endpoint field.

Checklist

Issue reference

KAG-4712

@samugi samugi changed the title Feat/otel formatted logs feat(observability): OpenTelemetry-formatted logs Jun 24, 2024
@samugi samugi changed the title feat(observability): OpenTelemetry-formatted logs feat(observability): OpenTelemetry logs Jun 24, 2024
@samugi samugi force-pushed the feat/otel-formatted-logs branch 2 times, most recently from 724a84a to a775404 Compare June 24, 2024 09:05
@samugi samugi added this to the 3.8.0 milestone Jun 24, 2024
@samugi samugi force-pushed the feat/otel-formatted-logs branch 3 times, most recently from 5e5257d to 3621b8f Compare June 24, 2024 17:22
kong/observability/logs.lua Show resolved Hide resolved
kong/observability/logs.lua Outdated Show resolved Hide resolved
kong/observability/logs.lua Outdated Show resolved Hide resolved
kong/observability/logs.lua Show resolved Hide resolved
kong/plugins/opentelemetry/handler.lua Outdated Show resolved Hide resolved
Copy link
Contributor

@ADD-SP ADD-SP left a comment

Choose a reason for hiding this comment

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

And also I suggest not rebase commits when applying reviewing suggestions, we can use the following command for better view for the reviewer.

git commit --fixup <commit_id>

@samugi
Copy link
Contributor Author

samugi commented Jun 25, 2024

And also I suggest not rebase commits when applying reviewing suggestions, we can use the following command for better view for the reviewer.

git commit --fixup <commit_id>

yes absolutely. I wasn't planning to rebase (for now), which will make syncing (via cherry-pick) with the existing EE PR easier as well.

kong/globalpatches.lua Show resolved Hide resolved


function _M.maybe_push(stack_level, log_level, ...)
-- WARNING: do not yield in this function, as it is called from ngx.log
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it is hard to detect yielding cheaply, let's keep this issue here for more discussion.

Copy link
Contributor Author

@samugi samugi Jun 26, 2024

Choose a reason for hiding this comment

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

I added a test: 5b6cef9

@samugi samugi requested a review from ADD-SP June 26, 2024 08:27
samugi added a commit to Kong/docs.konghq.com that referenced this pull request Jun 26, 2024
samugi added a commit to Kong/docs.konghq.com that referenced this pull request Jun 26, 2024
samugi added a commit to Kong/docs.konghq.com that referenced this pull request Jun 26, 2024
samugi added 10 commits June 27, 2024 10:30
Spans were not set as "active" during their lifetime.
This commit addresses the issue by setting each span as the current
"active" span in instrumentations. This is necessary in order to have
the correct span_id information in the OTel-formatted log entries.
When a disabled and non-request scoped hook was called outside of
a request scope, it was producing an error while trying to index ngx.ctx

This commit adds a check to early return from is_group_enabled when
outside of the request scope

This allows for hooks to still work when used in other contexts where
they can be enabled via the always_enable() function
This commit adds OpenTelemetry formatted logs as a new
signal supported by the OpenTelemetry plugin.

It patches the ngx.log function to collect the logged line and
enrich it with additional context info:

* severity
* introspection details
* timestamp

And for request-scoped logs also:

* Request ID
* Trace ID
* Span ID
* Trace flags

The log entries are protobuf-serialized and sent to the endpoint
configured in the OpenTelemetry plugin's config.logs_endpoint field.
This commit collects an additional tracing information (trace flags) in
the internal context and adds it to produced OTel-formatted log entries.
This commit moves the tracing and logging core code in the observability
folder.
This commit adds unit and integration tests for the OTel-formatted logs
feature.
feat(dynamic_hook): support non request-scoped hooks + disable

When a disabled and non-request scoped hook was called outside of
a request scope, it was producing an error while trying to index ngx.ctx

This commit adds a check to early return from is_group_enabled when
outside of the request scope

This allows for hooks to still work when used in other contexts where
they can be enabled via the always_enable() function

This commit also adds support for disabling hooks.
@samugi samugi force-pushed the feat/otel-formatted-logs branch 2 times, most recently from 95c7b8a to 8bdb8ed Compare June 27, 2024 08:49
@samugi
Copy link
Contributor Author

samugi commented Jun 27, 2024

no logic changes in recent commits, I've only fixed compat with older DPs and added tests.

@Achiel
Copy link
Contributor

Achiel commented Jun 27, 2024

@ADD-SP if possible could we get this reviewed on Monday? We have a wonderful set of customers that I'd love to show this off to as a beta. Thanks!

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.

None yet

4 participants