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

Align log sdk naming with api #2768

Merged
merged 6 commits into from
Sep 13, 2022

Conversation

jack-berg
Copy link
Member

Resolves #2752.

This aligns log SDK and API concepts which have diverged after the merged of #2676. This PR brings alignment to the log API and SDK, and in brings the log signal into alignment with tracing and metrics where there is conceptual overlap. There shouldn't be any new concepts introduced here.

  • Rename ../logs/logging-library-sdk.md to ../logs/sdk.md
  • Remove wording from SDK that implies that an API doesn't exist, like this.
  • Move How to Create Log4j Style Appender to api.md since it describes an API use case.
  • Move Implicit / Explicit Context Injection sections to api.md since they describe API level considerations.
  • Rename Logger create method to be emit, to align with SDK concept of LogRecordProcessor#onEmit(..).
  • Rename LogProcessor, LogExporter to LogRecordProcessor, LogRecordExporter.
  • Fill in various SDK level TODOs related to shutdown and flushing. The language from these was taken directly from the metrics / tracing SDK - no new concepts were introduced.

specification/logs/api.md Outdated Show resolved Hide resolved
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

spec-compliance-matrix.md Show resolved Hide resolved
specification/common/attribute-type-mapping.md Outdated Show resolved Hide resolved
specification/logs/api.md Outdated Show resolved Hide resolved
specification/logs/api.md Show resolved Hide resolved
specification/logs/api.md Outdated Show resolved Hide resolved
specification/logs/api.md Outdated Show resolved Hide resolved
@reyang reyang added spec:logs Related to the specification/logs directory area:sdk Related to the SDK labels Sep 2, 2022
specification/logs/api.md Outdated Show resolved Hide resolved
specification/logs/api.md Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member

I will keep this open for a bit to allow time to review the last changes.

@tigrannajaryan tigrannajaryan merged commit d710123 into open-telemetry:main Sep 13, 2022
Comment on lines +151 to +152
Events require the `event.domain` attribute. The API MUST not allow creating an
Event if the Logger instance doesn't have `event.domain` scope attribute.
Copy link
Member

Choose a reason for hiding this comment

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

@jack-berg @scheler

Events require the event.domain attribute. The API MUST not allow creating an
Event if the Logger instance doesn't have event.domain scope attribute.

Can you clarify for me what to do in the case of missing event.domain to the EmitEvent call? Is it a no-op or a throw? Do I generate a log to alert the user?

Also, is it valid if name is passed as null or empty?

Copy link
Contributor

Choose a reason for hiding this comment

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

@CodeBlanch both domain and name are required for an Event and also cannot be empty or null. So EmitEvent call should throw an error if event.domain attribute is missing or is null/empty. Same with name - if it is null or empty, EmitEvent should return failure or throw an error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sdk Related to the SDK spec:logs Related to the specification/logs directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consistency of components naming in logs SDK
9 participants