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

Requesting TC review for PHP Logs API/SDK spec compliance #1537

Closed
16 of 17 tasks
bobstrecansky opened this issue Jun 8, 2023 · 7 comments
Closed
16 of 17 tasks

Requesting TC review for PHP Logs API/SDK spec compliance #1537

bobstrecansky opened this issue Jun 8, 2023 · 7 comments
Assignees

Comments

@bobstrecansky
Copy link
Contributor

bobstrecansky commented Jun 8, 2023

Requesting a compliance review from https://github.com/orgs/open-telemetry/teams/technical-committee prior to GA release. If there are questions you can ask myself or any of the other PHP maintainers (@brettmc or @pdelewski)

[UPDATE from @tigrannajaryan] I am doing the review. Here is my checklist so far, borrowed mostly from compatibility matrix (checkmark means reviewed/verified against spec definition):

  • Getting Started example works
  • LoggerProvider.Get Logger
  • LoggerProvider.Get Logger accepts attributes
  • LoggerProvider.Shutdown
  • LoggerProvider.ForceFlush
  • Can get global LoggerProvider
  • Can set global LoggerProvider
  • Logger.Emit(LogRecord)
  • SimpleLogRecordProcessor (needs a rename)
  • BatchLogRecordProcessor (doesn't support async emitting), (needs a rename)
  • Can plug custom LogRecordProcessor
  • OTLP/gRPC exporter
  • OTLP/HTTP exporter
  • OTLP File exporter - Not yet implemented according to compatibility matrix.
  • Can plug custom LogRecordExporter
  • Trace Context Injection
  • LogRecord fields output
@carlosalberto
Copy link
Contributor

@tigrannajaryan

@tigrannajaryan
Copy link
Member

@open-telemetry/php-maintainers I finished working through my checklist. Other than a few bugs/clarification questions I filed as issues everything looks good to me.

Are there any other areas that you would like me to take a look?

@brettmc
Copy link

brettmc commented Jul 4, 2023

Thanks, @tigrannajaryan
I'm not confident about our handling of complex LogRecord attributes, I think it needs a closer look. In particular, we apply the "must be primitive or homogeneous array of primitives" check on them, but I suspect should not.

@tigrannajaryan
Copy link
Member

Thanks, @tigrannajaryan I'm not confident about our handling of complex LogRecord attributes, I think it needs a closer look. In particular, we apply the "must be primitive or homogeneous array of primitives" check on them, but I suspect should not.

@brettmc done and filed a couple bugs. If you can think of another review area please let me know.

@tigrannajaryan
Copy link
Member

@brettmc is there anything else that you would like me to take a look? If not I will close this issue as done.

@brettmc
Copy link

brettmc commented Jul 18, 2023

@tigrannajaryan no, nothing else. Thank you very much for the detailed review, it was very useful!
I have updated and closed the related issues, as I believe them to all be resolved now.

@tigrannajaryan
Copy link
Member

Closing as "done".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants