[chore] Create a single interface for the Receiver and Exporter parsers #2287
Conversation
Signed-off-by: Israel Blancas <[email protected]>
Signed-off-by: Israel Blancas <[email protected]>
Great idea! I'll do. Thanks! :) |
Signed-off-by: Israel Blancas <[email protected]>
There was a problem hiding this comment.
LGTM - May I ask:
- does it make sense to use the singular for the const names?
- Since it is now an int, does it make sense to introduce an unknown type to avoid unwanted defaults?
- Does it make sense to give the constant the
ComponentTypeprefix to avoid future name conflicts and make it a bit more expressive?
Signed-off-by: Israel Blancas <[email protected]>
There was a problem hiding this comment.
Just one change that I think would make this a bit cleaner, otherwise 👍
Overall, I desperately want real types to work with here. configFromString is particularly problematic here as it's preventing us from just putting these parsers on a type (receiver, exporter, etc.) which would mean we don't have to do any of the swapping on the desired component type.
|
|
||
| switch cType { | ||
| case ComponentTypeExporter: | ||
| compEnabled = GetEnabledExporters(logger, config) |
There was a problem hiding this comment.
Rather than switching on type here could we just call getEnabledComponents(cType, config)?
|
looks like a linting failure, will merge when the tests are ✅ |
Signed-off-by: Israel Blancas <[email protected]>
| @@ -123,6 +123,7 @@ func TestShouldInjectSidecar(t *testing.T) { | |||
| }, | |||
| }, | |||
| } { | |||
| tt := tt | |||
There was a problem hiding this comment.
I didn't detect the previous lint error because of the golang-ci version. I upgraded to the latest and now I'm getting a lot of Implicit memory aliasing in for loop errors.
I fixed them because we will have to do that in the future anyway.
There was a problem hiding this comment.
I thought that would only happen if we were using a reference to the test's memory address? Is this a known go issue for the testing library?
There was a problem hiding this comment.
It is something part of how Golang loops work. I see that for this specific case I’m not sure if we are in that situation but, since we’re in a loop… maybe we’re falling into that.
There was a problem hiding this comment.
In general, we do need this assignment. It doesn't matter unless we enable t.Parallel() for the test, but it's a good practice and probably why linters are complaining about it. Fundamentally, we're using the tt variable in a closure via t.Run, so if we ran the cases in parallel without reassigning, we'd get the same value in all of them. See https://go.dev/blog/loopvar-preview for a clearer and more detailed explanation.
There was a problem hiding this comment.
i see, i see. Once we upgrade to go 1.22 this will no longer be an issue though right?
There was a problem hiding this comment.
Once we upgrade the minimum Go version to 1.22 in our go.mod, yes. Just compiling with Go 1.22 isn't enough according to the blog post.
…rs (open-telemetry#2287) * Create a single interface for the Receiver and Exporter parsers Signed-off-by: Israel Blancas <[email protected]> * Fix lint Signed-off-by: Israel Blancas <[email protected]> * Use ComponentType Signed-off-by: Israel Blancas <[email protected]> * Apply changes requested in code review Signed-off-by: Israel Blancas <[email protected]> * Simplify getting the enabled components Signed-off-by: Israel Blancas <[email protected]> * Fix lint Signed-off-by: Israel Blancas <[email protected]> --------- Signed-off-by: Israel Blancas <[email protected]>
Description: Create a single interface for
ReceiverandExporterparsersLink to tracking Issue: #2008