feat(recombine): add combine_with option

[andrzej-stencel]

Fixes #314

[pmalek-sumo]

🚀

[codecov]

Codecov Report

Merging #315 (929671d) into main (2645572) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@          Coverage Diff          @@
##            main    #315   +/-   ##
=====================================
  Coverage   77.0%   77.0%           
=====================================
  Files         94      94           
  Lines       4408    4410    +2     
=====================================
+ Hits        3395    3397    +2     
  Misses       697     697           
  Partials     316     316           

Impacted Files	Coverage Δ
...perator/builtin/transformer/recombine/recombine.go	71.1% <100.0%> (+0.5%)	⬆️

[djaglowski]

@astencel-sumo This is a great improvement and I am fully in support of it.

That said, I think that we should have a test or two that demonstrate the configuration as it is read from an actual file, so that we can be sure there are not issues with the way a user would represent a newline character or any other character.

Would you mind adding a couple golden config tests to demonstrate the way a user would configure this?

[andrzej-stencel]

Thanks @djaglowski for pointing this out, will add the config tests.

Please not that for those to work, I will need this change: #317. Without it, I'm getting this failure:

$ go test
--- FAIL: TestConfig (0.00s)
    --- FAIL: TestConfig/default (0.00s)
        operatortest.go:85: 
                Error Trace:    operatortest.go:85
                                                        config_test.go:33
                Error:          Not equal: 
                                expected: &recombine.RecombineOperatorConfig{TransformerConfig:helper.TransformerConfig{WriterConfig:helper.WriterConfig{BasicConfig:helper.BasicConfig{OperatorID:"recombine", OperatorType:"metadata"}, OutputIDs:helper.OutputIDs(nil)}, OnError:"send", IfExpr:""}, IsFirstEntry:"", IsLastEntry:"", MaxBatchSize:1000, CombineField:entry.Field{FieldInterface:entry.FieldInterface(nil)}, CombineWith:"\n", OverwriteWith:"oldest"}
                                actual  : &recombine.RecombineOperatorConfig{TransformerConfig:helper.TransformerConfig{WriterConfig:helper.WriterConfig{BasicConfig:helper.BasicConfig{OperatorID:"recombine", OperatorType:"recombine"}, OutputIDs:helper.OutputIDs(nil)}, OnError:"send", IfExpr:""}, IsFirstEntry:"", IsLastEntry:"", MaxBatchSize:1000, CombineField:entry.Field{FieldInterface:entry.FieldInterface(nil)}, CombineWith:"\n", OverwriteWith:"oldest"}
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -5,3 +5,3 @@
                                     OperatorID: (string) (len=9) "recombine",
                                -    OperatorType: (string) (len=8) "metadata"
                                +    OperatorType: (string) (len=9) "recombine"
                                    },
                Test:           TestConfig/default
FAIL
exit status 1
FAIL    github.com/open-telemetry/opentelemetry-log-collection/operator/builtin/transformer/recombine   1.438s

[andrzej-stencel]

@djaglowski See my commit with config tests 8864cae.

[djaglowski]

Thanks for adding those config tests @astencel-sumo. Just one doc nit.

[djaglowski]

Nit - since a user should expect different results based on whether or not this value is quoted, I think we need to be precise here. Specifically, the user would have to specify "\n" in order to replicate the default behavior, so I think we should show that exact value here.

[andrzej-stencel]

Yes I fully agree, we should be explicit about the types of the values.

[djaglowski]

Typically, it doesn't matter if the user writes "foo" or foo. When read, the value becomes the same either way.

In this case, it does matter whether the user writes "\n" or \n. I think it is enough to show the value here that is actually interpreted the same as the default value.

Suggested change

	| `combine_with` | `\n` | The string that is put between the combined entries. This can be an empty string as well. |
	| `combine_with` | `"\n"` | The string that is put between the combined entries. This can be an empty string as well. |

[andrzej-stencel]

Fixed in 649bfc8.

Also updated examples in the docs, please take a look @djaglowski.

[djaglowski]

Thanks for the contribution @astencel-sumo. This is very nice improvement.

[andrzej-stencel]

Any plans for a new release @djaglowski? I'd like to use it in OTC's filelog receiver.

This change should probably also go into the Changelog, shouldn't it?

[djaglowski]

Yes, it should go in the changelog. I'll make a PR to prep the changelog for the release, and will cut a release as soon as that's merged, most likely can be today.