-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[mdatagen] Add log type definition #12822
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #12822 +/- ##
==========================================
- Coverage 91.55% 91.53% -0.02%
==========================================
Files 499 499
Lines 27102 27115 +13
==========================================
+ Hits 24814 24821 +7
- Misses 1809 1815 +6
Partials 479 479 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
default.log: | ||
enabled: true | ||
description: Example log record enabled by default. | ||
extended_documentation: The log record will be renamed soon. | ||
body: | ||
type: string | ||
attributes: | ||
- string_attr | ||
- boolean_attr | ||
warnings: | ||
if_enabled_not_set: This log will be disabled by default soon. |
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 understand why do we need to add this if it's not being used? What's the plan?
Also I'm still not convinced if we really need to provide framework for structured logs. How do you want to use it? Is there an existing use case in contrib?
Also why not events in that case? OTel events are supposed to be used as structured logs. See https://github.com/open-telemetry/opentelemetry-specification/blob/8bbac83c0c994c0a750cc688935fc1cae7fd39b3/oteps/0202-events-and-logs-api.md and other recent developments around that
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.
Also I'm still not convinced if we really need to provide framework for structured logs. How do you want to use it? Is there an existing use case in contrib?
We have a similar use case like this and personally I think this is a common use case for scraper based receivers as well.
Also why not events in that case? OTel events are supposed to be used as structured logs. See https://github.com/open-telemetry/opentelemetry-specification/blob/8bbac83c0c994c0a750cc688935fc1cae7fd39b3/oteps/0202-events-and-logs-api.md and other recent developments around that
Looks like events might be more appropriate for this use case. I'm not familiar with events and am I correct in thinking that logs and events are essentially similar, aside from some required fields? I'm open to implementing this with events instead of logs. @dmitryax What do you think?
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'm not familiar with events and am I correct in thinking that logs and events are essentially similar, aside from some required fields?
Yes, that should be correct. The event must always have a name field which would be a key in the metadata.yaml similar to a metric name.
I'm open to implementing this with events instead of logs. @dmitryax What do you think?
Yes, let's do it. I'd call this group in metadata.yaml events
instead of logs
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 have some additional questions about the implementation. We currently have a logs builder that activates if a receiver/scraper supports the logs
signal. With the introduction of an events
group in metadata.yaml
, do you think we need a separate events builder for this purpose? And would it be beneficial to add a logs.enabled
flag in metadata.yaml
to manage the generation of logs builder-related code?
I am considering implementing a single event builder that can accept an optional events group as a parameter, allowing it to offer basic functionality for logs and structured events. But it will probably make api users confused.
@dmitryax Do you have any suggestions for that?
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.
Given that events are essentially logs, I think we can keep the logs builder and just generate new methods for adding the structured events. For example having my.event
in the metadata.yaml events section would generate
func (lb *LogsBuilder) RecordMyEvent(...)
Let me know WDYT
Description
This PR added type definition needed for supporting structured logs in mdatagen.
Note:
logs: {}
tometadata.yaml
to enable basic log supportLink to tracking issue
Part of #12571
Testing
Added
Documentation
Added