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

Refine log fields #3804

Closed
jack-berg opened this issue Oct 28, 2021 · 10 comments
Closed

Refine log fields #3804

jack-berg opened this issue Oct 28, 2021 · 10 comments
Labels
Feature Request Suggest an idea for this project

Comments

@jack-berg
Copy link
Member

A handful of questions have come up regarding log fields. This issue tracks them so we don't forget.

  1. All the fields in the log proto are optional except timestamp, but only traceId, spanId, severityText, and Name are @Nullable. The other fields have default values. We should have a uniform strategy on this, or good reasons for breaking uniformity.
  2. There have been discussions about removing the setBody(Body body) method and simply keeping setBody(String message).
  3. There have been discussions about dropping or renaming the name field.
  4. We should make setFlags(int flags) more usable. Perhaps setFlags(LogFlags) akin to TraceFlags?
@jack-berg jack-berg added the Feature Request Suggest an idea for this project label Oct 28, 2021
@jkwatson
Copy link
Contributor

for 4), has the log SIG defined any flags yet? If not, we should probably just leave that out of our SDK until they do.

@jack-berg
Copy link
Member Author

On closer inspection, flags is actually trace flags.

There's an open spec issue to clarify its purpose.

@jkwatson
Copy link
Contributor

On closer inspection, flags is actually trace flags.

oh! Then we should definitely change our model to just accept TraceFlags and be done with it. :)

@anuraaga
Copy link
Contributor

Also to capture, an idea I'm quite fond of is having a LogEmitter.logBuilder().emit() type of pattern as opposed to LogEmitter.emit(Log.builder().build())

https://github.com/open-telemetry/opentelemetry-java/pull/3759/files#r731228356

@jkwatson
Copy link
Contributor

Also to capture, an idea I'm quite fond of is having a LogEmitter.logBuilder().emit() type of pattern as opposed to LogEmitter.emit(Log.builder().build())

https://github.com/open-telemetry/opentelemetry-java/pull/3759/files#r731228356

I'm definitely ok with starting with this API, and if someone really needs a direct interface to implement (maybe to support delegation to another logging data model, and hence zero-copy?), then it could easily be added in the future.

@jack-berg
Copy link
Member Author

I've included the change of the pattern to LogEmitter.logBuilder().emit() in PR #3759.

@jack-berg
Copy link
Member Author

I believe 2 is resolved now that spec PR #2096 was merged explaining why log body should be structured.

  1. There have been discussions about removing the setBody(Body body) method and simply keeping setBody(String message).

@anuraaga
Copy link
Contributor

anuraaga commented Nov 5, 2021

Would it make sense to leave the structured version out of the SDK until someone asks for it? It mentions compatibility with certain existing log formats, but perhaps we can wait until we know this actually exists in the Java ecosystem, unless someone already has an example.

@jack-berg
Copy link
Member Author

I'd be on board to make it an internal implementation detail until required. Can keep the setBody(String) method and add a setBody(Body) overload when requested?

@jack-berg
Copy link
Member Author

All but one of these items were addressed in #3837.

The last unaddressed item is about the log name field. This is being actively discussed in the specification. If the field is removed in the spec, I'll remove it here. But for now, I'll close this ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Suggest an idea for this project
Projects
None yet
Development

No branches or pull requests

3 participants