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

Issue 674 gcp logging spaninfo #677

Merged
merged 7 commits into from
Oct 2, 2024

Conversation

wabrit
Copy link
Contributor

@wabrit wabrit commented Sep 20, 2024

These changes address issue 674 by ensuring that the span id and sampled properties are set on the LogEntry.builder (without these settings it is not possible to exploit the feature of GCP console where a log entry can be used to launch inspection of the trace corresponding to that log (and vice versa).

These changes have been tested in a native-build Quarkus microservice (running Quarkus 3.11.3).

@wabrit wabrit requested a review from a team as a code owner September 20, 2024 13:57
@loicmathieu
Copy link
Collaborator

I'm not convinced about the labels, do they bring any added value? I understand that logback do this but for which purpose?

@wabrit
Copy link
Contributor Author

wabrit commented Sep 24, 2024

There was one general and one more specific reason I added the labels:

  • In a mixed economy of Spring Boot / Quarkus services running in GCP, having consistent GCP stuctured log layout makes inspecting cross-service logs (e.g. in the context of tracing) a little simpler. SB services typically use logback for their GCP logging, so this seemed a way to ensure some degree of consistency.
  • The loggerName label is probably the most useful of those 3. However when using JSON payload format, the same info and more is admittedly to be found in the jsonPayload.log.origin.class field, so the argument for this being repeated here is consistency at best in that when looking at cross-service logs you don't have to know that different components put this info in different places.

In short, I appreciate that there isn't a compelling technical reason for these labels, just a minor-ish usage one. If you'd prefer me to remove them, that's fine.

@loicmathieu
Copy link
Collaborator

loggerName seems legit, the other may be seen as too much duplication but maybe your rational makes sense.

Pinging @Fungrim as he was the original contributor of Google Cloud Logging extension, what do you think about the proposed default labels, does it makes sense?

@wabrit
Copy link
Contributor Author

wabrit commented Sep 25, 2024

if it helps I can split out the labels work into a separate PR, and leave this PR to focus on the tracing fix, as the latter is I think of far greater importance.

@Fungrim
Copy link
Contributor

Fungrim commented Sep 26, 2024

Thanks for the ping @loicmathieu, and the span is a nice catch @wabrit.

Regarding the labels and don't have any strong personal opinions. However, given the lack of standardization around log records and formats, I generally dislike hard coded values. If I did this change myself I would probably add a LogRecordLabelExtractor interface (much like the trace info extraction) that the user can bind via CDI to add whatever labels they want to the GCP record and/or expose these default values via a configuration option.

But again, these are not strong so I'm fine with the proposed changes as well.

@wabrit
Copy link
Contributor Author

wabrit commented Sep 26, 2024

The LogRecordLabelExtractor idea sounds much better than what I've done - thanks @Fungrim!

I'm happy to go either of the following ways:

  1. Modify this PR to include LogRecordLabelExtractor support
  2. Remove label code from this PR and submit a different PR with LogRecordLabelExtractor support

@loicmathieu let me know what you would prefer (FWIW my preference would be option (2) as I'd really like to get the trace fix in).

@loicmathieu
Copy link
Collaborator

@wabrit the best would be to do the 2. : remove the label change from this PR and provide a different PR.

@wabrit
Copy link
Contributor Author

wabrit commented Oct 2, 2024

@loicmathieu labels stuff now removed from PR

Copy link
Collaborator

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot

@loicmathieu loicmathieu merged commit 7ed7b11 into quarkiverse:main Oct 2, 2024
1 check passed
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

Successfully merging this pull request may close these issues.

3 participants