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

Mark LogRecord.CategoryName Obsolete #3493

Closed

Conversation

alanwest
Copy link
Member

Fixes #3491

This PR proposes that option 3 from #3491 is a good idea 💡.

  • LogRecord.CategoryName is marked obsolete
  • Console log exporter still exports CategoryName if it's populated
  • OTLP exporter ignores CategoryName
  • Introduces an attribute name serilog.source_context for the Serilog sink
  • The OpenTelemetryLogger now populates the dotnet.ilogger.category attribute instead of this being handled downstream

@alanwest alanwest requested a review from a team July 26, 2022 22:06
LogRecordAttributeList attributes = default;

// TODO: Confirm if this name for attribute is good.
attributes.Add("dotnet.ilogger.category", this.categoryName);
Copy link
Member

Choose a reason for hiding this comment

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

What's the perf drop (from both writer and reader perspective)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea agreed gotta measure this. Before I spend time on that though, I would like to first get a sense whether folks agree that obsoleting CategoryName is the route we want to take.

Copy link
Member

Choose a reason for hiding this comment

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

I put my quick thinking here #3491 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

Is this doing anything? 🤣 Seems to be adding the attribute to the stack and then dropping it on the floor. If we wanted to drop CategoryName & EventId here we would have to turn on the buffering feature so we could add new tags to AttributeStorage. Perf hit would be minor for users already doing buffering/batching but for users doing re-entrant processor to something like ETW (GenevaLogExporter) the perf hit would be significant!

I like @reyang's idea to just wait and see what else is coming and then tackle the weirdness. I would also love to drop LogRecord.State and only expose LogRecord.StateValues. Having the two is super confusing and it is kind of undefined what exporters should do with them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can't say I feel all that bad about my application of Cunningham's Law in this PR 😆. I am closing it because you've convinced me of the better option for now: #3491 (comment)

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

Need to understand the perf.

@alanwest
Copy link
Member Author

#3493 (comment)

@alanwest alanwest closed this Jul 29, 2022
@alanwest alanwest deleted the alanwest/logrecord-categoryname branch July 29, 2022 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decide what to do with LogRecord.CategoryName
3 participants