Retry refactor to allow for batching to retry.

[nicklas-dohrn]

Description

This change mostly changes around, where the retry logic is applied.
Thinking about the things that need to be retried, this is likely to only be needed for networking.
So the logic of retrying should be done after the serialisation of the data for every protocol, to increase performance and locality of problem solutions.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Testing performed?

• Unit tests
• Integration tests
• Acceptance tests

Checklist:

• This PR is being made against the main branch, or relevant version branch
• I have made corresponding changes to the documentation
• I have added testing for my changes

If you have any questions, or want to get attention for a PR or issue please reach out on the #logging-and-metrics channel in the cloudfoundry slack

[ctlong]

I have a number of stylistic concerns that I've left as comments throughout the code.

In general your approach seems sound. However, I would really like to see some unit tests added for this retry logic in order to validate that it performs as expected.

[ctlong]

Why embed *Retryer rather than using a named field to encapsulate the new struct via composition?

[ctlong]

I think that I would prefer composition in this case because I don't see a good reason to exposes the fields and methods of *Retryer directly.

[nicklas-dohrn]

I was using a Pointer here, so that I could insert the retryer by creating it in the writer factory layer, so that the implementation and logic does not rely on the writers itself to propagate the settings through to the retryer.
Might be a shortcoming due to my limited understanding of Go idiomatic concepts.

[ctlong]

Why not just represent Retryer as some new fields and methods in HTTPSBatchWriter since that's the only writer set to use it?

[ctlong]

I don't have a strong opinion on this, but I think I lean toward just putting all this code into HTTPSBatchWriter so that it's all more self-contained.

[nicklas-dohrn]

I was thinking about that as well, but when one would introduce the retryer in the way I built it here, it would have more than one benefit:

1. Retrying after stringifying the rfsyslog message seemed beneficial performance wise.
2. Having a connection aware retry logic for tls based approaches might result in a more efficient approach to solve connectivity issues (retries are only needed due to connectivity issues)
3. the retry_writer retries parsing issues for syslog messages #612 is also present for tls/tcp writers. this could be fixed with a dedicated pr.