Skip metrics/traces with refactor #105
Conversation
internal/transform/suites.go
Outdated
| Meter(string) metric.Meter | ||
| } | ||
|
|
||
| func TransformAndLoadSuites(ctx context.Context, cfg *config.Config, provider OtelProvider, |
There was a problem hiding this comment.
nit: is the function name duplicating the package name? Having to use it like this transform.TransformAndLoadSuites seems weird. The natural AndLoadSuites sounds incorrect, although not sure about WithLoadSuites. wdyt?
There was a problem hiding this comment.
I will think about this on my walk and adjust the name when I get back.
|
|
||
| import "os" | ||
|
|
||
| type FileReader struct { |
There was a problem hiding this comment.
question: if this is used only in tests, do we need it to be exposed, even under the internal package?
There was a problem hiding this comment.
In the future we may use it for more than tests. If a switch is added to take a file path we can use it for that.
I agree. I almost made a separate PR. I wanted to also have the skip metric/trace tests to demonstrate why the |
|
At some point I'm going to want to add an interface mocker to improve test coverage. Any objections or preferences on which? |
ryanrolds
force-pushed
the
rolds/refactor_for_better_testing
branch
from
11567df to
723d5c6
Compare
|
I made some adjustments that I think addresses the concerns with the naming. |
|
@ryanrolds I've not forgotten this 🙏 I'm at Gophercon SG returning back to Spain next Monday. |
|
Friendly nudge. |
|
@ryanrolds I'm sorry for the delay, finished Gophercon but I was on PTO this week. Nevertheless, I'm reviewing it right now. |
| "github.com/joshdk/go-junit" | ||
| ) | ||
|
|
||
| type InputReader interface { |
There was a problem hiding this comment.
suggestion: let's use a more descriptive name, as our implementations (File and Stdin) will read and produce from jUnit.
| type InputReader interface { | |
| type JunitInputReader interface { |
| // Skip sending traces to the OpenTelemetry collector | ||
| SkipTraces bool | ||
| // Skip sending metrics to the OpenTelemetry collector | ||
| SkipMetrics bool |
There was a problem hiding this comment.
question: do we want to skip one, the other or both, or we just want to skip sending to the collector, with just one flag?
I have no strong opinion, but I'm concerned about the growing number of flags, for the long term.
| const defaultMaxBatchSize = 10 | ||
| const propertiesAllowAll = "all" | ||
| const Junit2otlp = "junit2otlp" |
| Meter(string) metric.Meter | ||
| } | ||
|
|
||
| func ExtractTransformAndLoadReport(ctx context.Context, cfg *config.Config, |
There was a problem hiding this comment.
suggestion: let's add function comments with the explanation 🙏
| } | ||
| } | ||
|
|
||
| func NewProvider(ctx context.Context, cfg *config.Config) (*OtelProvider, error) { |
There was a problem hiding this comment.
suggestion: let's add comments here too
| @@ -1,4 +1,4 @@ | |||
| package main | |||
| package otel | |||
There was a problem hiding this comment.
question: do we want these keys to be internal, or a public reference for others to define testing values for their own projects?
|
@ryanrolds friendly nudge 😄 |
Here is a purposed implementation of skip metrics/traces with a refactoring of the project structure and introduction of a config package. The refactor made adding the tests easier and will make implementing tests easier in the future.
Please let me know what you think. I'm happy to adjust it.
Major changes
internal.intenal/configpackage to handle configurationMaintoRunTestReadertoFileReaderFakeGitRepothat required the samples existing in the same dir as the test and the root of the repo