logstash-logback-encoder icon indicating copy to clipboard operation
logstash-logback-encoder copied to clipboard

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

Open waschmittel opened this issue 3 years ago • 8 comments

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

waschmittel avatar Apr 11 '22 07:04 waschmittel

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

elefeint avatar Apr 11 '22 15:04 elefeint

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 avatar Apr 13 '22 10:04 brenuart

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.

elefeint avatar Apr 13 '22 13:04 elefeint

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)

philsttr avatar Apr 13 '22 16:04 philsttr

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.

elefeint avatar Apr 13 '22 17:04 elefeint

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.

philsttr avatar Apr 13 '22 18:04 philsttr

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

elefeint avatar Apr 13 '22 18:04 elefeint

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.

waschmittel avatar Apr 14 '22 17:04 waschmittel

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

waschmittel avatar Sep 26 '22 13:09 waschmittel

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

brenuart avatar Sep 26 '22 15:09 brenuart

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.

elefeint avatar Sep 30 '22 14:09 elefeint

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.

github-actions[bot] avatar Oct 11 '22 13:10 github-actions[bot]