-
Notifications
You must be signed in to change notification settings - Fork 187
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
Add event.summary as a recommendation for events #1074
Conversation
f2a4fca
to
f55bff4
Compare
Events are identified by an `event.name` and a set of attributes and fields in the body that carry specific meaning. However, since these events will be combined with other logs, `event.message` allows a backend to display a human-readable representation of the event.
f55bff4
to
02c0631
Compare
Proposed PR for issue #1076 As discussed in the Eventing SIG on 2024-05-24 cc: @MSNev @trask @JamieDanielson |
model/registry/event.yaml
Outdated
@@ -16,3 +16,13 @@ groups: | |||
separation of semantics for events in separate domains like browser, mobile, and | |||
kubernetes. | |||
examples: ['browser.mouse.click', 'device.app.lifecycle'] | |||
- id: message |
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.
maybe event.description
?
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 took message
as it's a convention that already exists... but granted it could be confusing with messaging.: description
is not bad idea.
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.
Respectfully, I don't think we should do this.
Events are identified by an event.name and attributes and fields in the body that carry specific meaning.
Yes. Events have defined semantics and attributes are spec'd to convey those semantics.
However, since these events will be combined with other logs, event.message allows a backend to display a human-readable representation of the event.
The fact that Events happen to be an extension of Logs should be irrelevant here. Users and consumers of Events should not be concerned with the underlying transport mechanism, and there are APIs to help treat events as a legitimate, separate signal type.
Why are we trying to allow a backend to display something human readable? Backends don't typically display much anyway. It sounds like this use case is already covered by structured logging, and I don't think we should muddy Events with a message
field whose human-readable content could be misleading or otherwise conflict the semantics of the event itself.
Maybe the definition of backend is not clear and could be better specified. I meant as backend logging backends (like DataDog, Google Logging, Elastic, ... those are the systems I have experience with), because they display the logs to the user: For us (as an end-user) it's important to mix our normal logs without events in the same stream. We use the convention of We defined our internal business event based ontop of structured logs with success (as we mandate the |
Maybe an extra note: I'm afraid that if we cannot leverage the fact that logs are also meant for human consumption, for events, my feeling on the fact that events are built on top of logs will flip to unsupportive (we will keep on using our events based on structured logs). Then, it would probably have been better to make logs their own signal. At least then, if that signal had a message/description in the proto, at least a log-backend could also consume those signals and do something with it. |
42a1401
to
3ac8d4a
Compare
3ac8d4a
to
23efdca
Compare
@MSNev I've removed the workflow started example, and added examples parallel to the examples in |
I think I could be talked into this if there was wording to say something like "If the |
one potential concern with the name |
As asked in the Eventing SIG, I'll add a few redacted events with its message/summary/description: Event: import:dgc_asset_import {
"message": "Exported 1057 nodes from 166 roots in 0.051s",
"event_class": "generic",
"event_name": "export:dgc_asset_export",
"root_nodes_count": 166,
"authenticated_id": "REDACTED",
"duration": 51,
"output_format": "JSON",
"root_resource_type": "terms",
"c4_container": "backend",
"max_page_nodes_count": 1057,
"send_notification": false,
"query_limit": 200,
"resource_types": [
"vocabulary",
"terms",
"attributes",
"source",
"type",
"status",
"target"
],
"timestamp": "2024-06-07T19:11:44.563Z",
"data_size": 404475,
"trace_id": "8853d755645a95f3809341f2c41ed92c",
"output_type": "TEXT",
"all_nodes_count": 1057,
"span_id": "bcad170d7e71c0d0",
"count_limit": 10001,
"query_type": "TABLE_VIEW_CONFIG",
"validation_enabled": false,
"request_source": "JAVA_API",
"valid_query": true,
"c4_system": "knowledgegraph",
"status": "INFO"
} Event: import:dgc_asset_import {
"message": "Asset import completed with result: SUCCESS",
"operation_type": "IMPORT",
"community_by_mapping_count": 0,
"simulation": false,
"resources_updated": {
"MP": 5
},
"job_state": "COMPLETED",
"authenticated_id": "REDACTED",
"duration": 25,
"community_by_name_count": 0,
"domain_by_id_count": 0,
"asset_by_name_count": 0,
"domain_by_mapping_count": 0,
"file_type": "JSON",
"job_result": "SUCCESS",
"send_notification": false,
"continue_on_error": false,
"timestamp": "2024-06-07T19:11:44.568Z",
"commands_executed": 0,
"trace_id": "bcdfe64836f084dac3edd79b33546fd4",
"span_id": "6d3936b98a8181e4",
"community_by_id_count": 0,
"domain_by_name_count": 0,
"asset_by_mapping_count": 5,
"request_source": "REST_API",
"commands_skipped": 0,
"save_result": false,
"asset_by_id_count": 0,
"service": "dgc",
"job_id": "REDACTED",
"event_class": "generic",
"event_name": "import:dgc_asset_import",
"commands_count_in_request": 7,
"errors_count": 0,
"status": "INFO"
} Event: catalogjdbc:edge_jdbc_connect {
"message": "connection to the datasource has been established",
"database_name": "REDACTED",
"timing": {
"total_calls": 2,
"time_unit": "MILLISECONDS",
"max_time": 16994,
"mean_time": 8606,
"total_time": 17212
},
"schema_connection_id": "REDACTED",
"schema_name": "REDACTED",
"driver_class_name": "REDACTED",
"duration": 17212,
"tmp_msg": "",
"database_id": "REDACTED",
"engine": "JAVA_API",
"service": "jdbc-ingestion",
"schema_id": "REDACTED",
"event_name": "catalogjdbc:edge_jdbc_connect",
"event_class": "generic",
"correlation_id": "REACTED",
"status": "INFO",
"timestamp": "2024-06-07T19:27:10.826Z"
} |
Thanks for the examples, @alexvanboxel . I think these do a good job of demonstrating what these are for, and I think they also demonstrate:
I will remove my blocking request, because I think there isn't too much harm in this, but unfortunately I cannot offer a supportive approval. I ask that other reviewers please consider some phrasing that discourages the use of the message/description/summary for well-specified otel events. I also still think that an attribute is a reasonable place to put a summary of the event, and we don't need it to be first-class Event field. |
I've said my peace. Others can continue discussing.
1952be4
to
8f19d75
Compare
Thanks for the feedback. I've added a section that I hope formulates your concerns. I've also renamed everything to |
Yes, I think |
does the same issue exist for how to display a human-readable representation of a structured log? |
Had a discussion about the One question is whether the content of the summary attribute should be strictly defined as part of the semantic convention for the event. We (Me, Nev, Trask) feel that the value should not be strictly defined. Like For example, a commonly defined database event in a particular library may want to include a library-specific summary. An end user tracking down a production problem may flag a particular failure or exception as suspicious and mention some context specific information in the summary. Because the content of this attribute is not intended to be indexed and used as part of machine analysis, strictly defining its content is not necessary. This is different from the Span Personally, I would prefer |
Events are identified by an `event.name` and a set of attributes and fields in the body that carry specific | ||
meaning. However, since these events will be combined with other logs, `event.summary` allows a | ||
centralized logging system to display a human-readable representation of the event. | ||
|
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.
This paragraph does not seem to provide new information - it's already captured in the event name, the markdown, and the brief. Could it be removed?
Events are identified by an `event.name` and a set of attributes and fields in the body that carry specific | |
meaning. However, since these events will be combined with other logs, `event.summary` allows a | |
centralized logging system to display a human-readable representation of the event. |
When summaries are generated, they are not expected to include every attribute and field that is part of the | ||
event but could contain those that are meaningful for a human operator when visually navigating a centralized | ||
log system. Instrumentation libraries that produce events defined within the standard OpenTelemetry are not | ||
expected to add an `event.summary` attribute, as these are well-known events. |
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.
Could we make it normative? E.g.
When summaries are generated, they are not expected to include every attribute and field that is part of the | |
event but could contain those that are meaningful for a human operator when visually navigating a centralized | |
log system. Instrumentation libraries that produce events defined within the standard OpenTelemetry are not | |
expected to add an `event.summary` attribute, as these are well-known events. | |
It's NOT RECOMMENDED to record full string representation of the event on the `event.summary` attribute. The summary SHOULD be concise and human-readable as it's expected to be used by a human operator when visually navigating a centralized log system. |
I removed
Instrumentation libraries that produce events defined within the standard
OpenTelemetry are not expected to add anevent.summary
attribute, as these are well-known events.
I'm not sure how it could work - if we have 100+ events defined, no backend would be able to special-case each event and define human-readable representation of it. I.e. I don't understand why we would NOT allow to define a summary for at least some standards events.
I would also agree to not making it strongly typed and instead would propose an attribute for the outcome as suggested in #1089 |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
@alexvanboxel Do you want this reopened? |
yes, please. I've been on holiday and couldn't (we'll it holiday) pick this up. I can pick it up again. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
In looking at this again, my understanding of the general consensus so far is that having
Is there anything else I am missing that needs to be resolved for this? |
# Use pipe (|) for multiline entries. | ||
subtext: | | ||
Events are identified by an `event.name` and a set of attributes and fields in the body that carry specific | ||
meaning. However, since these events will be combined with other logs, `event.description` allows a |
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.
Should be event.summary
The biggest question for me is should the summary be strongly typed ie an enum or free text |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Changes
Events are identified by an
event.name
and attributes and fields in the body that carry specific meaning. However, since these events will be combined with other logs,event.summary
allows a backend to display a human-readable representation of the event.Merge requirement checklist
[chore]