Skip to content

otelslog: set SeverityText on log records#7198

Merged
pellared merged 8 commits into
open-telemetry:mainfrom
adomaskizogian:main
Apr 18, 2025
Merged

otelslog: set SeverityText on log records#7198
pellared merged 8 commits into
open-telemetry:mainfrom
adomaskizogian:main

Conversation

@adomaskizogian
Copy link
Copy Markdown
Contributor

@adomaskizogian adomaskizogian commented Apr 10, 2025

Sets SeverityText using slog.Level available at the source.

Not having SeverityText is somewhat awkward when scouting through logs. I believe majority of software maintainers are used to having severity expressed as string.
Plus otelzap does it already.

Resolves:

@adomaskizogian adomaskizogian requested review from a team, MrAlias and pellared as code owners April 10, 2025 21:12
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented Apr 10, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Copy Markdown
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Please add a changelog entry and cover the new functionality in unit tests?

Comment thread bridges/otelslog/handler.go Outdated
@adomaskizogian
Copy link
Copy Markdown
Contributor Author

Please add a changelog entry and cover the new functionality in unit tests?

Thanks for feedback. Will do

@MrAlias
Copy link
Copy Markdown
Contributor

MrAlias commented Apr 10, 2025

The linked issues are already closed as we do not plan to add this. It seems the only justification being given here is subjective and a guess at best. Is there other motivation not already provided?

@adomaskizogian
Copy link
Copy Markdown
Contributor Author

adomaskizogian commented Apr 11, 2025

The linked issues are already closed as we do not plan to add this. It seems the only justification being given here is subjective and a guess at best. Is there other motivation not already provided?

In none of those issues the intent to not support SeverityText on slog bridge in the future was expressed.

In both of those issues though, the need for it was stated.
Under one of the comments it was mentiomed that zap bridge already supports it and I followed a similar approach, used it as an example.

One of the motivators is poor ux when querying for logs and filtering by severity. Now not having the easy and quick to grasp SeverityText will force everyone to do the seveirty matching using numeric representation which for someone outside of otel domain can be not so intuitive compared to a text representation of it. E.g. Local dev environment with console exporter configured. Chanves are users will have to add log record transformation pipelines to introduce it.
Why not solve it like was done on otel zap?

I strongly believe different bridges should be on par feature wise and otel bridge should not influence the choice of logging library

Comment thread bridges/otelslog/handler.go Outdated
@dmathieu
Copy link
Copy Markdown
Member

If we do this, it will need to be tested.

@pellared
Copy link
Copy Markdown
Member

pellared commented Apr 11, 2025

The linked issues are already closed as we do not plan to add this. It seems the only justification being given here is subjective and a guess at best. Is there other motivation not already provided?

I reopened the issue. See: #6707 (comment).

@pellared pellared changed the title [Slog Bridge] Set SeverityText on log records otelslog: set SeverityText on log records Apr 11, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.0%. Comparing base (e2db05c) to head (301de44).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #7198   +/-   ##
=====================================
  Coverage   81.0%   81.0%           
=====================================
  Files        204     204           
  Lines      18079   18080    +1     
=====================================
+ Hits       14650   14651    +1     
  Misses      3003    3003           
  Partials     426     426           
Files with missing lines Coverage Δ
bridges/otelslog/handler.go 93.3% <100.0%> (+<0.1%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@adomaskizogian
Copy link
Copy Markdown
Contributor Author

Please add a changelog entry and cover the new functionality in unit tests?

changelog entry: bd0284e

unit tests: 2acc745

@pellared pellared added enhancement New feature or request bridge: slog Related to the slog bridge labels Apr 14, 2025
@pellared pellared requested a review from dmathieu April 14, 2025 11:18
@pellared
Copy link
Copy Markdown
Member

Planning to merge on Friday.
CC @MrAlias

@pellared pellared merged commit 3bb8502 into open-telemetry:main Apr 18, 2025
@pellared
Copy link
Copy Markdown
Member

@adomaskizogian, thank you very much for you contribution 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bridge: slog Related to the slog bridge enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants