Logging API and SDK modifications #12
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ranton256
reviewed
Dec 1, 2020
| @@ -59,47 +58,51 @@ class Logger | |||
| * A default LogRecord that will be assigned if no parameters are passed to Logger's .log() method | |||
| * which should at minimum assign the trace_id, span_id, and timestamp | |||
| */ | |||
| virtual void log(const LogRecord &record) noexcept = 0; | |||
| virtual void Log(LogRecord &record) noexcept = 0; | |||
ranton256
reviewed
Dec 1, 2020
api/test/logs/logger_test.cc
Outdated
| logger->log(r); | ||
| r.name = "Log Record"; | ||
| r.severity = Severity::kInfo; | ||
| logger->Log(r); |
There was a problem hiding this comment.
Verify what log record actually gets created by Log() methods?
ranton256
reviewed
Dec 1, 2020
|
|
||
| /** | ||
| * Writes a log record into the processor. | ||
| * @param record The record to write into the processor. | ||
| */ | ||
| void log(const opentelemetry::logs::LogRecord &record) noexcept override; | ||
| void Log(opentelemetry::logs::LogRecord &record) noexcept override; |
ranton256
reviewed
Dec 1, 2020
sdk/src/logs/logger.cc
Outdated
| // Inject values into record if not user specified | ||
| // Timestamp | ||
| if (r->timestamp == opentelemetry::core::SystemTimestamp(std::chrono::seconds(0))) | ||
| r->timestamp = core::SystemTimestamp(std::chrono::system_clock::now()); |
There was a problem hiding this comment.
For consistency with other single line if statements recommend using { and }
kxyr
reviewed
Dec 1, 2020
| auto logger = lp->GetLogger("TestLogger"); | ||
|
|
||
| // GetLogger(name, args) function | ||
| std::array<string_view, 1> sv{"string"}; |
There was a problem hiding this comment.
Yup it's still in but the GitHub diff tool said that it was taken out and readded somewhere else... confusing
| std::array<string_view, 1> sv{"string"}; | ||
| span<string_view> args{sv}; | ||
| auto logger2 = lp->GetLogger("TestLogger2", args); | ||
| ASSERT_EQ("test logger", logger->GetName()); |
There was a problem hiding this comment.
Add ASSERT_EQ(logger2->GetName(), "TestLogger2")
There was a problem hiding this comment.
The API implementation returns a noop instance, so the name would always be "noop logger" which is what the first test checks for
kxyr
reviewed
Dec 1, 2020
| @@ -39,8 +39,7 @@ class Logger | |||
| virtual ~Logger() = default; | |||
|
|
|||
| /* Returns the name of the logger */ | |||
| // TODO: decide whether this is useful and/or should be kept, as this is not a method required in | |||
| // the specification. virtual nostd::string_view getName() = 0; | |||
| virtual nostd::string_view GetName() noexcept = 0; | |||
…try-cpp into logs-API-SDK-update
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses many of the comments we received when we PRed the initial Logging API and initial Logging SDK.
GetName()method to logger API and SDK.Note this PR relies on #403 to be merged to compile.
cc - @alolita @xukaren