Proposing a formatter allow-list configuration

[eyalkoren]

It's easy to imagine cases where users would want some logs to be automatically reformatted, but not others.
We can choose different entities to filter upon, for example - loggers, but the formatting entity seems like a good candidate to provide a good default, as normally we would like to reformat plain-text or pattern-based formatters, but not format specific ones, like JSON/HTML/XML etc.

@basepi I already added this config to the Java agent, but not necessarily we want to align on that. I asked your review because you are the only one that implemented it so far, to see if you think it makes sense.

[apmmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2021-08-18T04:23:19.151+0000

• Duration: 3 min 4 sec

• Commit: f45a84c

Trends 🧪

[basepi]

I don't think this would apply for Python. The Python implementation is a very blunt instrument, we explicitly recommend users to do it manually for better control.

We may want to bring in other agent teams to see if this makes sense as a cross-agent option or if it should just be Java.

[eyalkoren]

Checking with other agents - if no agent see how this may be relevant, we don't have to include it in the spec, it can be Java-agent-specific

[trentm]

I think it would be a very rare, but possible, use case in Node.js-land. The following are limiters to a config like this being used by Node.js users:

• Only a subset of the logging frameworks supported by ecs-logging-nodejs support multiple "transports/formatters" and only one of the three (Winston) discusses having multiple transports without literal "stern warning"s against doing so.
• An implementation of this for Node.js would be a little bit of hack, because matching the allow list patterns would be looking at the constructor names of the transport/formatter objects passed into the logging framework API. That works, but isn't the kind of thing I see done much in node.js packages, so might be somewhat surprising to users.
• Like @basepi mentioned for Python, I think I would tend to encourage Node.js APM + ecs-logging users to explicitly setup their logging config manually because there can be unintended side-effects.

[eyalkoren]

Like @basepi mentioned for Python, I think I would tend to encourage Node.js APM + ecs-logging users to explicitly setup their logging config manually because there can be unintended side-effects.

Just to clarify - the entire purpose of log_ecs_reformatting is to make this functionality zero-config as part of the onboarding enhancement efforts we make. Wherever we feel it's not possible or not safe, we need to think if we really want to implement that and recommend users to do otherwise. The ECS-logging libraries already offer minimal manual config effort, this one is really reducing from minimal to zero.

[eyalkoren]

An additional thing I just noticed now that I missed in my original review - we called this config log_ecs_reformatting in the Java agent, whereas in the spec and the Python agent it's log_ecs_formatting.

Personally, I prefer reformatting because it emphasizes the "aggressive" nature of this instrumentation, but I don't mind changing it if others prefer formatting. @basepi , assuming this was not intentional, WDYT?

[basepi]

@eyalkoren I don't have strong feelings on log_ecs_formatting vs log_ecs_reformatting. I do think the latter is probably more descriptive. This feature is still marked as experimental so it's easy enough to change in python.

[russcam]

This looks fine to me, although not relevant to the .NET agent.

[eyalkoren]

OK, I will merge this with a clarification that this is currently only used in the Java agent, so it is there in case some other agent needs something similar when implementing this.
Thanks for the feedback 🙏