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

Supporting Logging API .Log() Overloads and GetName() method #422

Merged
merged 7 commits into from
Dec 20, 2020

Conversation

MarkSeufert
Copy link
Contributor

@MarkSeufert MarkSeufert commented Dec 5, 2020

This PR adds .Log() overloads to the Logging API, as well as a GetName() method. More specifically for the overloads, the following was added:

  • There are now 6 Log overloads: Log(Severity, message), Log(Severity, name, message), Log(Severity, KeyValue object), Log(Severity, name, KeyValue object), Log(Severity, initializer list) and Log(Severity, name, initializer list).
  • Each of the above overloads has 6 additional methods that automatically set the severity for it: .Trace(), .Info(), .Debug(), .Warn(), .Error(), and .Fatal().

These methods make it easier for users to write simple logs using the API, since they won't have to populate the 9 argument logging statement.

cc - @alolita @xukaren

@MarkSeufert MarkSeufert requested a review from a team December 5, 2020 00:56
@codecov
Copy link

codecov bot commented Dec 5, 2020

Codecov Report

Merging #422 (78fc630) into master (c8054c5) will decrease coverage by 0.02%.
The diff coverage is 96.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #422      +/-   ##
==========================================
- Coverage   94.38%   94.35%   -0.03%     
==========================================
  Files         185      185              
  Lines        8014     8047      +33     
==========================================
+ Hits         7564     7593      +29     
- Misses        450      454       +4     
Impacted Files Coverage Δ
api/test/logs/provider_test.cc 100.00% <ø> (ø)
sdk/include/opentelemetry/sdk/logs/logger.h 100.00% <ø> (ø)
sdk/src/logs/logger.cc 65.90% <0.00%> (-3.14%) ⬇️
api/include/opentelemetry/logs/logger.h 100.00% <100.00%> (ø)
api/include/opentelemetry/logs/noop.h 100.00% <100.00%> (ø)
api/test/logs/logger_test.cc 91.48% <100.00%> (-3.11%) ⬇️
exporters/ostream/test/ostream_log_test.cc 100.00% <100.00%> (ø)
sdk/test/logs/logger_sdk_test.cc 88.88% <100.00%> (-0.31%) ⬇️

api/include/opentelemetry/logs/log_record.h Outdated Show resolved Hide resolved
api/include/opentelemetry/logs/log_record.h Outdated Show resolved Hide resolved
api/include/opentelemetry/logs/logger.h Show resolved Hide resolved
api/include/opentelemetry/logs/logger.h Show resolved Hide resolved
api/include/opentelemetry/logs/logger.h Outdated Show resolved Hide resolved
api/include/opentelemetry/logs/logger.h Outdated Show resolved Hide resolved
kxyr pushed a commit to open-o11y/opentelemetry-cpp that referenced this pull request Dec 7, 2020
kxyr pushed a commit to open-o11y/opentelemetry-cpp that referenced this pull request Dec 8, 2020
kxyr pushed a commit to open-o11y/opentelemetry-cpp that referenced this pull request Dec 8, 2020
kxyr pushed a commit to open-o11y/opentelemetry-cpp that referenced this pull request Dec 8, 2020
api/include/opentelemetry/logs/log_record.h Outdated Show resolved Hide resolved
api/include/opentelemetry/logs/logger.h Outdated Show resolved Hide resolved
@kxyr
Copy link
Contributor

kxyr commented Dec 10, 2020

The Recordable implementation for logs should simplify the overloads here. This PR will be coming in the next few days. Would be good to update/review again this PR after the Recordable is merged.

kxyr pushed a commit to open-o11y/opentelemetry-cpp that referenced this pull request Dec 12, 2020
kxyr pushed a commit to open-o11y/opentelemetry-cpp that referenced this pull request Dec 13, 2020
@MarkSeufert MarkSeufert force-pushed the logs-api-update branch 2 times, most recently from 2253df3 to ca24b29 Compare December 16, 2020 05:49
Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

Fixes with Recordable look great!

api/include/opentelemetry/logs/log_record.h Outdated Show resolved Hide resolved
api/include/opentelemetry/logs/log_record.h Outdated Show resolved Hide resolved
api/include/opentelemetry/logs/log_record.h Outdated Show resolved Hide resolved
Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

LGTM

@lalitb
Copy link
Member

lalitb commented Dec 18, 2020

@MarkSeufert Can you please rebase this branch with base, so that it is good to merge.

@maxgolov maxgolov added the pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.) label Dec 18, 2020
@lalitb lalitb merged commit d787758 into open-telemetry:master Dec 20, 2020
kxyr pushed a commit to open-o11y/opentelemetry-cpp that referenced this pull request Dec 22, 2020
kxyr pushed a commit to open-o11y/opentelemetry-cpp that referenced this pull request Dec 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants