quarkus-google-cloud-services icon indicating copy to clipboard operation
quarkus-google-cloud-services copied to clipboard

Add GCP operations logging

Open Fungrim opened this issue 3 years ago • 11 comments

This PR suggests a new module which adds direct integration with GCP Operations logging using the GCP client library directly in order to support tracing, labeling, etc.

Copy 'n pasted from #224:

The ideas is:

  • Log directly to GCP using the Google Java library
  • Support both structured and unstructured logging
  • The log record instant and level should be first class citizen regardless
  • Support labels for things like POD name, app name etc (GCP logging labels acts as "dimensions" when analyzing logs)
  • Support trace ID as a first class citizen (to be able to link GCP traces with logs)
  • Default to ECS format for structure logging, but make it customizable

Fungrim avatar Jan 03 '22 09:01 Fungrim

@loicmathieu Good point, I'll add an injected logger to the test as well.

Fungrim avatar Jan 04 '22 09:01 Fungrim

PR is updated, but as noted above the itest fails silently. I have no idea why, but since Logging.flush hangs I'm assuming it has something to do with the gRPC or Netty libs.

Fungrim avatar Jan 04 '22 20:01 Fungrim

@loicmathieu Sorry about the long silence. But I've managed to have a new look now and figured out why no logs where uploaded: In the logging runtime I'm injecting the Google credentials (in LoggingProducer), however the main integration test is using mocked credentials from ApplicationProducerMock and thus no logs are sent to Google Operations.

(I had looked at the secret manager implementation, but failed to realize the integration test for secret manager uses a mocked client as well).

Fungrim avatar Feb 03 '22 16:02 Fungrim

Yes, I use the main IT in two ways:

  • Manual testing against a real GCP project, I usually launch the project using quarkus:dev after updating the config to use my project ID and a service account then do curl to manually test (using GoogleServicesResourcesTest.java should also do the trick).
  • Automated testing via the other test cases inside the it test package

For GCP logging, you should manually test it (I'll also do it before merging your branch) and add a test method inside the GoogleServicesResourcesTest.java.

loicmathieu avatar Feb 04 '22 09:02 loicmathieu

Right. I'll do that. However, I also fetched/merged upstream to 1.1.0-SNAPSHOT and that broke JUL and JBoss logging. Curiously, injecting a Slf4j logger still works (as it hits the QuarkusDelayedHandler) so something has changed in Quarkus. Let me know if you want to me to close this PR until I've figured it out or if we should keep it open.

Fungrim avatar Feb 04 '22 10:02 Fungrim

You can keep it open, if I have time I'll have a look next week.

The change was from Quarkus 2.5 to Quarkus 2.7, something must have disturbed the loggin system between the two. Maybe JNDI that is now disabled by default.

loicmathieu avatar Feb 04 '22 11:02 loicmathieu

Thanks for the hint. I'll dig around a bit and see what I can find. I'll ping you when I've fixed it - apart from that I'm pretty happy with the state of the code at the moment.

Fungrim avatar Feb 04 '22 14:02 Fungrim

I also need to do a full review as there is a lot of code and I didn't check everything. On top of my head there is a lot of functionalities and some complexities, I wonder if we need all of them (now). I'll try to take some time for a full review next week, if I didn't did it at the end of the week just ping me ;) It wil be cool to have it merged for the next release but as I just make one this is not urgent.

loicmathieu avatar Feb 04 '22 14:02 loicmathieu

I miiiight have gotten a bit carried away with features :-D

Fungrim avatar Feb 04 '22 14:02 Fungrim

I have good news and bad news. The good one is that the extension seems to work as it should now. The bad? I have no idea why. I was in the middle of digging through the class path and attempting to downgrade jboss-logging (which had been updated in Quarkus 2.7) when... er... it magically started working. I have no exact explanation, but it might be that I'd forgotten to do a clean/install on the entire project between versions. I'm assuming there's at least a possibility that there could have been class version errors in pre-compiled code that caused it, but I'm really just guessing. Embarrassing, but, hey...

So, it looks good now. Going through it again there's one class - EscJsonFormat - that is a bit gnarly and completely untested. I'll see if I can sit down and do that this weekend. Otherwise I'm feeling pretty good about it now.

Fungrim avatar Feb 04 '22 21:02 Fungrim

@loicmathieu Thanks for the review! I appreciate your comment about the feature set, and I certainly acknowledge that smaller PR with a prior discussion would have been better.

As for the selection of the features, here's some comments that might clarify why it ended up as a fairly big chuck of code: I had two concrete goals when I started (based on the work I'm doing professionally with GCP): integrate tracing with with the logs in GCP Operations - this meant that only formatting was not an option, I needed to go via the GCP logger itself. And also I wanted structured logging - this could have been accomplished with logging-json, but since I wanted tracing it would mean a round-trip from JSON to string and back again (since the GCP logger works on a Map of objects) which I didn't like. ECS seemed simple enough to implement.

(Then having started I realized the ability to configure/specify labels is a rather big USP when integrating directly with the GCP logger - so I added that, the rest is history).

Fungrim avatar Feb 19 '22 08:02 Fungrim

@Fungrim do you still want to contribute this feature ? If will you have time to end this ? If not may I take your contribution and end it myself ?

loicmathieu avatar Jan 02 '23 11:01 loicmathieu

@Fungrim as I didn't receive any update from you I guess you don't have time to finish it so I'll take it over. Anyway, thanks for the work you did, very appreciated !

loicmathieu avatar Jan 31 '23 13:01 loicmathieu