Remove '$' from field syntax

[djaglowski]

This is a possible resolution to #164. This would be a major breaking change, so if accepted, the timing of a merge should be considered carefully.

---

This library has a construct called 'field', which has the
purpose of providing a concise way to refer to values in a
log record. The special symbol $ has been used in key
words that refer to top level fields in the log record.
i.e. $body, $attributes, $resource

Additionally, some shorthand notion was supported. Notably,
$.foo and .foo were equivalent to $body.foo.

This change proposes to remove the usage of $ in field syntax.
The main idea here is that all usages of field syntax MUST
begin with a key word i.e. body, attributes, resource.
With this requirement built in, there is no need to differentiate
between top level fields and arbitrary keys. The distinction is
implicit because the first word is always a top level field and
the remainder are arbitrary nested keys.

[codecov]

Codecov Report

Merging #364 (81157ca) into main (a523052) will increase coverage by 0.0%.
The diff coverage is 97.2%.

@@          Coverage Diff          @@
##            main    #364   +/-   ##
=====================================
  Coverage   75.2%   75.3%           
=====================================
  Files         83      83           
  Lines       4033    4036    +3     
=====================================
+ Hits        3035    3041    +6     
+ Misses       696     694    -2     
+ Partials     302     301    -1     

Impacted Files	Coverage Δ
operator/transformer/remove/rootable_field.go	62.5% <ø> (ø)
entry/field.go	95.1% <66.6%> (-2.5%)	⬇️
entry/attribute_field.go	100.0% <100.0%> (ø)
entry/body_field.go	100.0% <100.0%> (ø)
entry/resource_field.go	100.0% <100.0%> (ø)
operator/helper/expr_string.go	86.4% <100.0%> (ø)
operator/transformer/flatten/flatten.go	62.9% <100.0%> (ø)
operator/transformer/retain/retain.go	81.0% <100.0%> (ø)
operator/transformer/recombine/recombine.go	77.7% <0.0%> (+2.0%)	⬆️
... and 1 more

[djaglowski]

@jsirianni, @pmm-sumo, @rockb1017, please take a look. Any reservations on this approach?

[tigrannajaryan]

@dmitryax please have a look. Will our helm chart be affected by this change?

[tigrannajaryan]

The change looks good to me. The syntax becomes clearer.

I do not see any downsides to this change, are there any?

[pmm-sumo]

This is a large set of changes, I agree it improves the syntax actually. I asked @pmalek-sumo for another look as well (he was commenting on the issue)

[djaglowski]

The change looks good to me. The syntax becomes clearer.

I do not see any downsides to this change, are there any?

I don't see any downsides either.

[pmalek-sumo]

I like the idea of making it more explicit and easier to reason about.

🚀 some nits in .md tables

[dmitryax]

@dmitryax please have a look. Will our helm chart be affected by this change?

It will be affected, but we can adopt for this change.

I like the proposal. The syntax seems cleaner now

[djaglowski]

The contrib test failures are expected due to them using fields that are implicitly body fields.

[dmitryax]

LGTM, but shouldn't we throw deprecation warnings in logs?

[djaglowski]

LGTM, but shouldn't we throw deprecation warnings in logs?

Unfortunately, there's not a great way to complain about the deprecated usage because it is processed during unmarshaling, where we don't have a handle to a logger. We could add a flag to the Field struct, set it during unmarshaling, and then log every time the field is used in the pipeline, but this seems overkill to me. In my opinion, logging about deprecations is a nice-to-have feature and probably not worth any substantial code changes to do it. If you feel strongly though, I can make the above change, or perhaps you see a better way that I missed.

Another option is to fully remove support, but to detect the old syntax and return a clear error. This forces migration immediately, but at least would not be ambiguous at all.

[dmitryax]

Maybe we can drop the old interface support here but do all the necessary translation from the deprecated format in the collector-contrib? There we can throw all the warnings during the translation.

[djaglowski]

Unfortunately, I don't think unmarshaling in collector-contrib gives us a better opportunity to do this.

[djaglowski]

@dmitryax, can you clarify whether you see the lack of deprecation warning as a blocker?

I do not see a good way to log warnings here or at the collector level, so I would prefer to either error or quietly support the old syntax.

[dmitryax]

What if we return an error of a DeprecationError type from unmarshal functions? Then contrib can look for that error and throw a warning message to the output with everything else working as usual.

[djaglowski]

What if we return an error of a DeprecationError type from unmarshal functions? Then contrib can look for that error and throw a warning message to the output with everything else working as usual.

I liked this idea and gave it a shot, but unfortunately, the yaml library does not appear to allow for partial failures like this. Any error returned ultimately triggers a call to fail.

[dmitryax]

I see, if it's not possible to show a warning, I don't think silently support both versions is useful. Changing the interface right away seems to be a better option then.