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

log tracing #2446

Merged
merged 2 commits into from
Aug 23, 2023
Merged

log tracing #2446

merged 2 commits into from
Aug 23, 2023

Conversation

swalchemist
Copy link
Contributor

@swalchemist swalchemist commented Aug 17, 2023

This implements log tracing, where log messages will now include a trace ID and span ID that's either sent in from the API client or otherwise auto-generated. This allows for improved debugging, so transactions can be correlated between different web services that use the same trace ID. In the example below, - [ace36bc5bdccffef,ace36bc5bdccffef] is the part that is added:

[2023-08-16T00:56:46.060135Z] uaa - 13 [https-jsse-nio-8443-exec-1] - [ace36bc5bdccffef,ace36bc5bdccffef] .... DEBUG --- UaaMetricsFilter: Successfully matched URI: /oauth/token to a group: /oauth-oidc

In some code paths, like startup and rate limiting where the filter that implements log tracing isn't run, the trace and span IDs may be blank:

[2023-08-17T01:53:42.790149Z] uaa/uaa - 17490 [main] - [,] .... INFO --- SpringSecurityCoreVersion: You are running with Spring Security Core 5.7.10

See also a related change for uaa-release to update the log4j format there.

NOTE: This could be a breaking change for any stakeholder who parses UAA logs, because this add information to the middle of each log line in uaa.log.

[#185000950]

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/185849157

The labels on this github issue will be updated when the story is started.

@hsinn0
Copy link
Contributor

hsinn0 commented Aug 17, 2023

A message that I posted in a private slack thread before:

Regrading the logging format for trace-id & span-id, I wonder if we should do the similar to what spring-cloud-sleuth does instead. spring-cloud-sleuth does logging as follows by default for example:

2023-06-27 17:19:11.024 INFO [,80f198ee56343ba864fe8b2a57d3eff7,e457b5a2e4d86bd1] 66074 --- [nio-8080-exec-1] c.e.hellosleuth.HelloSleuthApplication : Hello world!

The format is having [service-name-value,trace-id-value,span-id-value] at the beginning.

Has this been considered?

@swalchemist
Copy link
Contributor Author

Regarding the logging format for trace-id & span-id, I wonder if we should do the similar to what spring-cloud-sleuth does instead. spring-cloud-sleuth does logging as follows by default for example:

2023-06-27 17:19:11.024 INFO [,80f198ee56343ba864fe8b2a57d3eff7,e457b5a2e4d86bd1] 66074 --- [nio-8080-exec-1] c.e.hellosleuth.HelloSleuthApplication : Hello world!

There were tradeoffs wherever we put it. It seems that if someone is parsing the logs, they might have a pattern that looks at the time stamp and the fields following it. So putting the new information just before "----" seemed slightly less likely to break any existing patterns. In fact, there was an acceptance test in uaa-release that had a pattern that still worked, while moving the trace ID earlier would have broken it.

But I get what you've said about the scope of the trace ID being broader than the thread ID (and even the uaa app name). It's hard to compare with the spring-cloud-sleuth example because other things are already in a different order, like the log level.

Ultimately, though, I don't feel strongly about where we place the new information, and we just made what thought was the least undesirable choice at the time.

@bruce-ricard bruce-ricard force-pushed the 185000950-trace-logs-impl branch from 41dd3cc to ea83bd4 Compare August 17, 2023 21:56
peterhaochen47 and others added 2 commits August 17, 2023 18:02
- based on this example app using brave lib (https://github.com/openzipkin/brave-example/tree/59215c36ce8ec12cbb2358fd6e7e6b494bc42b29/webmvc4-boot)
- aside from the brave docs and example app, we also have to manually add the TracingFilter bean to the filter chain

[#185000950]

Co-authored-by: Hongchol Sinn <[email protected]>
Co-authored-by: Danny Faught <[email protected]>
* This was copied from sample code, doesn't seem to be needed for UAA.
@bruce-ricard bruce-ricard force-pushed the 185000950-trace-logs-impl branch from ea83bd4 to 28fb175 Compare August 17, 2023 22:02
@Tallicia
Copy link
Contributor

Tallicia commented Aug 22, 2023

@strehle @torsten-sap In reviewing the comments and discussions, I am inclinded to merge this. Do you feel there is a roadblock that needs to be resolved before merging?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious why these changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The diff is not very helpful. We just inserted one line and changed the position numbers on the rest.

Copy link
Contributor

@Tallicia Tallicia left a comment

Choose a reason for hiding this comment

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

LGTM

@Tallicia Tallicia merged commit 2af7ad0 into develop Aug 23, 2023
@Tallicia Tallicia deleted the 185000950-trace-logs-impl branch August 23, 2023 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in_review The PR is currently in review
Projects
Development

Successfully merging this pull request may close these issues.

5 participants