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

Review logger Implementation for the stable log data model. #1291

Closed
lalitb opened this issue Mar 28, 2022 · 9 comments · Fixed by #1652
Closed

Review logger Implementation for the stable log data model. #1291

lalitb opened this issue Mar 28, 2022 · 9 comments · Fixed by #1652
Assignees
Labels
do-not-stale spec-compliance Not compliant to OpenTelemetry specs

Comments

@lalitb
Copy link
Member

lalitb commented Mar 28, 2022

Log Data Model specs are marked as stable - open-telemetry/opentelemetry-specification#2387. It would be a good time to review the logger api and sdk implementation and validate if the existing log structure can be mapped to this data model. Also, how to continue supporting fields/attributes that are not part of this logger model - eg log-name.

@lalitb lalitb added the spec-compliance Not compliant to OpenTelemetry specs label Mar 28, 2022
@lalitb lalitb self-assigned this Mar 28, 2022
@lalitb lalitb removed their assignment May 5, 2022
@owent
Copy link
Member

owent commented May 7, 2022

I have a quick look at opentelemetry-proto to v0.17.0 and the main branch of logging-library-sdk in opentelemetry-specification, and I want to confirm and disscuss the changes.

  • InstrumentationLibrary is deprecated and we should use InstrumentationScope now.This will also changes the trace and metrics. Should we add a option to switch between legacy InstrumentationLibrary and new InstrumentationScope?
    • instrumentation_library_spans should be replaced by scope_spans
    • instrumentation_library_metrics should be replaced by scope_metrics
    • instrumentation_library_logs should be replaced by scope_logs
  • LogProcessor::OnReceive(LogRecordable) should be renamed to LogProcessor::Emit(LogData)
  • Rename LoggerProvider to LogEmitterProvider
  • Rename LoggerContext to LogEmitter and move InstrumentationScope from Logger to LogEmitter
  • Rename LogRecord to LogData

Am I missed anything?

@esigo
Copy link
Member

esigo commented May 7, 2022

Hi @owent, thanks for looking into this.
I'd vote for not having a switch for supporting legacy.

@owent
Copy link
Member

owent commented May 7, 2022

Without supporting legacy,there will be break changes to SDK and the output of OTLP exporters.Especially the trace APIs are already stable, is it acceptable?

@esigo
Copy link
Member

esigo commented May 7, 2022

Sure we will have breaking changes here. I'd still vote for not supporting legacy. We are already low on resources, and supporting both legacy and new proto will make maintenance more complicated for us. We can discuss this in the next SIG meeting. I'll update you.
cc @lalitb @ThomsonTan

@lalitb
Copy link
Member Author

lalitb commented May 7, 2022

Thanks, @owent for summarising the changes required.

@lalitb
Copy link
Member Author

lalitb commented Jun 1, 2022

#1383 closes it.

@lalitb lalitb closed this as completed Jun 1, 2022
@owent
Copy link
Member

owent commented Jun 2, 2022

@lalitb This issue has not be finished completely.I'm waiting for #1413 and then continue to implement LogEmitter, there may be more conflicts if I do it now.

@lalitb
Copy link
Member Author

lalitb commented Jun 2, 2022

Yes my bad, we still need to implement LogEmitter. Reopening this issue.

@owent owent reopened this Jun 2, 2022
@github-actions
Copy link

github-actions bot commented Aug 2, 2022

This issue was marked as stale due to lack of activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-stale spec-compliance Not compliant to OpenTelemetry specs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants