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

Should "Emit a LogRecord" API set Timestamp if unspecified? #3386

Closed
Tracked by #2911
tigrannajaryan opened this issue Apr 12, 2023 · 7 comments
Closed
Tracked by #2911

Should "Emit a LogRecord" API set Timestamp if unspecified? #3386

tigrannajaryan opened this issue Apr 12, 2023 · 7 comments
Assignees
Labels
spec:logs Related to the specification/logs directory

Comments

@tigrannajaryan
Copy link
Member

tigrannajaryan commented Apr 12, 2023

The Timestamp field is optional in the Emit a LogRecord API and in the data model.

However, is there a reasonable expectation that by default all log records are generated "now" and should be stamped with the current time?

Should we set Timestamp to the current time if it is not specified?

The one opposing argument I can think of is that somehow the user obtains the log record externally, and the external source does not include the information about when the log record was created. In this case keeping the Timestamp unspecified is desirable, to indicate that it is unknown. Here is what the data model says about the Timestamp field:

Time when the event occurred measured by the origin clock, i.e. the time at the source. This field is optional, it may be missing if the source timestamp is unknown.

If we do not by default set the Timestamp to the current time then it is the responsibility of the caller who is implementing the bridge to always set the Timestamp. This sounds like a reasonable requirement to me.

This behavior would be surprising for end-user facing logging API, but this is not an end-user facing logging API. It is optimized for Bridges and the preference is likely that we are able to explicitly indicate that the timestamp is not known.

The discussion triggered by this comment.

@tigrannajaryan tigrannajaryan added the spec:logs Related to the specification/logs directory label Apr 12, 2023
@tigrannajaryan
Copy link
Member Author

@open-telemetry/specs-logs-approvers @reyang FYI.

@tigrannajaryan tigrannajaryan changed the title Should "Emit a LogRecord" API set Timestamp if unspecified Should "Emit a LogRecord" API set Timestamp if unspecified? Apr 12, 2023
@jack-berg
Copy link
Member

It is optimized for Bridges and there the preference is likely that we are able to explicitly indicate that the timestamp is not known.

I don't feel strongly about this, but we could default timestamp to the current time while also supporting the case of explicitly indicating that the timestamp is not known by requiring that users specify timestamp=0 when unknown.

@trask
Copy link
Member

trask commented Apr 12, 2023

If we do not by default set the Timestamp to the current time then it is the responsibility of the caller who is implementing the bridge to always set the Timestamp. This sounds like a reasonable requirement to me.

👍 I think logging libraries will have typically already captured this timestamp, and it seems like best practice would be to pass that along

@tigrannajaryan
Copy link
Member Author

It is optimized for Bridges and there the preference is likely that we are able to explicitly indicate that the timestamp is not known.

I don't feel strongly about this, but we could default timestamp to the current time while also supporting the case of explicitly indicating that the timestamp is not known by requiring that users specify timestamp=0 when unknown.

I think this is an opportunity to demonstrate that we stick to the "Bridge API is not for end users" message. :-)

I don't want the bridge author have harder time figuring out how to specify an unknown timestamp so that someone else who is not implementing a bridge can have nicer defaults.

@tigrannajaryan
Copy link
Member Author

If we do not by default set the Timestamp to the current time then it is the responsibility of the caller who is implementing the bridge to always set the Timestamp. This sounds like a reasonable requirement to me.

👍 I think logging libraries will have typically already captured this timestamp, and it seems like best practice would be to pass that along

Yes, exactly. If you are implementing a bridge/appender you already have a timestamp that was captured for you. You just need to pass that along. For bridge authors there is no value in having a default of "now".

@jack-berg
Copy link
Member

I think this is an opportunity to demonstrate that we stick to the "Bridge API is not for end users" message. :-)

👍 I'm on board with requiring timestamp be explicitly set.

@tigrannajaryan
Copy link
Member Author

@open-telemetry/specs-logs-approvers I am resolving this as "won't do" (i.e. we will leave Timestamp unspecified if the caller does not specify it).

Please comment if you feel this is not the right decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:logs Related to the specification/logs directory
Projects
None yet
Development

No branches or pull requests

4 participants