Keep only two routers, one for docker and another for crio/containerd

[VihasMakwana]

See #819.

We can use two routers as, crio and containerd are similar
The recombine operator is same for both crio and containerd, so we can use just one.

This also fixes an probable issue signalfx/splunk-otel-collector-chart#644

[Allex1]

@VihasMakwana thanks for your contribution .Please follow our guide in order for the ci to pass.

[TylerHelmuth]

@VihasMakwana can please share how you tested this change to be equivalent to the current solution of using 2 routers? @djaglowski can you share your thoughts on the changes to the filelogreceiver configuration?

[VihasMakwana]

So, for this, I have tested it the following way:

• For docker-based containers, I have tested them using minikube and splunk_hec to verify the timestamps and event delivery

• For crio and containerd-based containers, I have tested them using minikube and microk8s, and splunk_hec to verify the timestamps and event delivery and covered the following scenarios:

  • Used zulu timestamp and verified that it was being received with no errors
  • Used timestamp with local offset (rfc3393 spec) and verified that it was being received with no errors

[djaglowski]

I agree that the recombine operators for crio vs containerd are redundant and can be consolidated, but I do not understand the other changes.

At a glance the regex changes do not look correct to me. Can you give some specific examples that demonstrate how "^[^ Z]+ " and "^[^ Z]+Z" are correctly combined into "^[^ ]+ "?

[VihasMakwana]

@djaglowski please check signalfx/splunk-otel-collector-chart#644.
I made changes to regex because it was hard-coded to accept zulu.

My bad, forgot to mention this in PR description ;(

[VihasMakwana]

"^[^ ]+ " matches the timestamp.
It will match 2023-01-23T12:09:28.344779078-06:00 and also, 2023-07-06T13:08:23.702Z.
"^[^ Z]+Z" only matches the zulu timestamp and "^[^ Z]+ " only matches non-zulu i.e. with timezone offset.

[VihasMakwana]

Also, 2006-01-02T15:04:05.999999999Z07:00 will be able to parse both, zulu and non-Zulu ones.

[djaglowski]

Thanks for explaining @VihasMakwana. I think your explanation of the regexes makes sense but I haven't tested them so I'd appreciate others to look at this as well.

Personally I find it confusing that there is a bug fix in the same PR as non-functional changes. Both changes would be simpler to understand if presented in isolation. Perhaps you'd consider splitting the PR.

Finally, if these changes are accepted here, would you please submitting equivalent PRs to the contrib example here?

[VihasMakwana]

Makes sense.
But the issue is about keeping two routers, and for that to accomplish the changes in regex is required. And that also fixes the issue I attached in previous comment.

[VihasMakwana]

and yes, will submit PRs to contrib.

[TylerHelmuth]

@VihasMakwana have users of signalfx/splunk-otel-collector-chart gotten the chance to use these changes you've applied to that chart? This is a unique situation where we have the ability to confirm something in the field before merging it and I'd like to take advantage of that.

[VihasMakwana]

Hmm, I have an open PR for the similar thing in splunk repo, maybe we can wait until that one gets accepted and tested?

[povilasv]

This looks fine to me

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[VihasMakwana]

^^

[atoulme]

LGTM. Can you fix the lint issue?

[VihasMakwana]

@atoulme done

[TylerHelmuth]

@VihasMakwana have you had a chance to release this in the Splunk helm chart and confirm it is equivalent?

[VihasMakwana]

@TylerHelmuth No, not yet. If you or anyone else can test this out on your end, it would be of great help ;).

[VihasMakwana]

@atoulme can you re-review this one?

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.