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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Add support for library instrumentation #979

Merged
merged 9 commits into from
Jun 25, 2022
Merged

feat: Add support for library instrumentation #979

merged 9 commits into from
Jun 25, 2022

Conversation

losalex
Copy link
Contributor

@losalex losalex commented Jun 21, 2022 •
edited

This feature provides an ability to log extra entry with diagnostics structure which contains logging library information. Such entry is logged only once when first entry is written by a process using logging library.

Fixes #978, 751 馃

@losalex losalex requested review from a team as code owners June 21, 2022 19:30
@product-auto-label product-auto-label bot added size: l Pull request size is large. api: logging Issues related to the googleapis/java-logging API. labels Jun 21, 2022
minherz
minherz reviewed Jun 23, 2022
View reviewed changes
Copy link
Contributor

@minherz minherz left a comment •
edited

Choose a reason for hiding this comment

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

I did not find instrumentation for the calls to Logging.Write() and Logging.WriteAsync(). Are they expected to be in a separate PR? I recommend to instrument all paths before merging.

google-cloud-logging/src/main/java/com/google/cloud/logging/Instrumentation.java Outdated Show resolved Hide resolved
google-cloud-logging/src/main/java/com/google/cloud/logging/Instrumentation.java Outdated Show resolved Hide resolved
google-cloud-logging/src/main/java/com/google/cloud/logging/Instrumentation.java Show resolved Hide resolved
google-cloud-logging/src/main/java/com/google/cloud/logging/Instrumentation.java Show resolved Hide resolved
google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingHandler.java Outdated Show resolved Hide resolved
minherz
minherz approved these changes Jun 24, 2022
View reviewed changes
Copy link
Contributor

@minherz minherz left a comment

Choose a reason for hiding this comment

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

there is a case of NPE that should be handled. Left few more comments for considerations. Beside that it is good to go.

google-cloud-logging/src/main/java/com/google/cloud/logging/Instrumentation.java Show resolved Hide resolved
google-cloud-logging/src/main/java/com/google/cloud/logging/Instrumentation.java Show resolved Hide resolved
google-cloud-logging/src/main/java/com/google/cloud/logging/Instrumentation.java Show resolved Hide resolved
google-cloud-logging/src/main/java/com/google/cloud/logging/Instrumentation.java Outdated Show resolved Hide resolved
@losalex losalex added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 25, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 25, 2022
@losalex losalex merged commit 2749974 into main Jun 25, 2022
@losalex losalex deleted the losalex/feat-instrumentation branch June 25, 2022 07:36
@release-please release-please bot mentioned this pull request Jun 25, 2022
Merged
@losalex losalex mentioned this pull request Jun 27, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@minherz minherz minherz approved these changes

Assignees
No one assigned
Labels
api: logging Issues related to the googleapis/java-logging API. size: l Pull request size is large.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@losalex @minherz @yoshi-kokoro