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

Making SingleFieldAppendingMarker#getFieldValue() package-private broke log assertions #788

Closed
waschmittel opened this issue Apr 11, 2022 · 12 comments
Labels
status/waiting-for-feedback Waiting for feedback from the original author. Issue will be closed if feedback is not received soon

Comments

@waschmittel
Copy link

waschmittel commented Apr 11, 2022

Description

I am maintaining https://github.com/dm-drogeriemarkt/log-capture/ which we use to test our logging.

I also allows to assert logging of keyValue, see https://github.com/dm-drogeriemarkt/log-capture/#key-value-from-logstash

Making SingleFieldAppendingMarker#getFieldValue() package-private broke that for me, because I used it for those assertions. I don't see a viable replacement, although I might be overlooking something.

Expected behavior

I am aware that is kind of a https://xkcd.com/1172/ situation ;-), but I would like a way to be still able to use those assertions. If everything else fails, I can mis-use toStringSelf() and filter out the key, but I'd rather not do that.

  • logstash-logback-encoder 7.1
@elefeint
Copy link

Was ObjectAppendingMarker.getFieldValue() never meant to be public API? We relied on it, perhaps incorrectly, to enrich logging data with Logstash markers.

@waschmittel waschmittel changed the title Making SingleFieldAppendingMarker#getFieldValue() broke log assertions Making SingleFieldAppendingMarker#getFieldValue() package-private broke log assertions Apr 11, 2022
@brenuart
Copy link
Collaborator

brenuart commented Apr 13, 2022

Hi @elefeint

The visibility of getFieldValue() has indeed been reduced - it wasn't meant to be part of the API and should ideally remain internal to the class hierarchy.

As you know, we also have more complex markers like MapEntriesAppendingMarker and ObjectFieldAppendingMarker in addition to the SingleFieldAppendingMarker... If you tell me more about your usage scenario may be we could find a more powerful approach that would give you access to the information contained in the other markers as well..

@brenuart brenuart added the status/waiting-for-feedback Waiting for feedback from the original author. Issue will be closed if feedback is not received soon label Apr 13, 2022
@elefeint
Copy link

The enhancer is an optional add-on that adds Logstash markers as log metadata for logs written to GCP Cloud Logging. So as long as we have a way to extract marker information from logback's ILoggingEvent, we don't need to rely on the marker's internal implementation details.
For background, GoogleCloudPlatform/spring-cloud-gcp#198 is the feature request for which this enhancer was implemented.

@philsttr
Copy link
Collaborator

philsttr commented Apr 13, 2022

Does spring-cloud-gcp logging allow appending arbitrary "raw" JSON to their JSON event output? If so, it could:

  1. create a JsonGenerator that writes to a string,
  2. call logstashMarker.writeTo(JsonGenerator)
  3. append the string as "raw" JSON to their JSON event output

This approach would support every type of LogstashMarker (not just the implementations of SingleFieldAppendingMarker)

@elefeint
Copy link

spring-cloud-gcp glues together Logback and GCP abilities -- in this case, logback's JsonLayout. JsonLayout accepts a map of keys and values, but not raw JSON.

@philsttr
Copy link
Collaborator

That's unfortunate. The limitations of JsonLayout are one of the reasons logstash-logback-encoder exists ;)

The LogstashMarkers weren't really designed to be consumed by anything other than the encoders within logstash-logback-encoder. This issue is the first case I've heard about where LogstashMarkers are being consumed by something other than the encoders within logstash-logback-encoder. As such, the current implementation in spring-cloud-gcp that consumes the markers directly is going to be extremely fragile.

Will need to think about this some to come up with a good approach.

@elefeint
Copy link

Thank you! For now, we'll hold off on upgrading.

@waschmittel
Copy link
Author

waschmittel commented Apr 14, 2022

Fwiw, we also need the actual value and not JSON for our assertions @philsttr, since the whole point of those assertions is that they are based on content and should work regardless of log formatting/encoding.

So I would also be happy if there was a way to get to the contents.

@github-actions github-actions bot removed the status/waiting-for-feedback Waiting for feedback from the original author. Issue will be closed if feedback is not received soon label Apr 14, 2022
@waschmittel
Copy link
Author

Not an issue for me anymore. I've removed the assertion from my log testing library.

@brenuart
Copy link
Collaborator

@waschmittel Thanks for the feedback.
@elefeint Are you still impacted by this issue?

@brenuart brenuart added the status/waiting-for-feedback Waiting for feedback from the original author. Issue will be closed if feedback is not received soon label Sep 26, 2022
@elefeint
Copy link

Yes, but because it's an optional add-on feature, we could remove it in a major release in early 2023.
It's an understandable mismatch in targeted use-cases. Your readme does say that markers are only supported "if using LogstashEncoder/Layout"!

Ultimately, end users can create custom GCP logger enhancers with the data that they use for logstash markers.

@github-actions
Copy link

This issue has been automatically closed because there has been no response to our request for more information from the original author. With only the information that is currently in the issue, we don't have enough information to take action. Please reach out if you have or find the answers we need so that we can investigate further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/waiting-for-feedback Waiting for feedback from the original author. Issue will be closed if feedback is not received soon
Projects
None yet
Development

No branches or pull requests

4 participants