sdk/log: Add TestRecord #5200
sdk/log: Add TestRecord #5200
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5200 +/- ##
=====================================
Coverage 84.3% 84.3%
=====================================
Files 258 258
Lines 16974 16978 +4
=====================================
+ Hits 14313 14317 +4
Misses 2362 2362
Partials 299 299
|
pellared
requested review from
MrAlias,
Aneurysm9,
evantorrie,
XSAM,
dashpole,
MadVikingGod,
hanyuancheung and
dmathieu
as code owners
pellared
added
pkg:SDK
| // TestRecord is a log record for testing. | ||
| // You can use it for unit testing [Processor] and [Exporter] implementations. | ||
| // Do not use TestRecord in production code. | ||
| type TestRecord struct { |
There was a problem hiding this comment.
How do I get a Record from a TestRecord so I can use it for testing?
There was a problem hiding this comment.
.Record. it is used in stdoutlog test in this PR
| // TestRecord is a log record for testing. | ||
| // You can use it for unit testing [Processor] and [Exporter] implementations. | ||
| // Do not use TestRecord in production code. | ||
| type TestRecord struct { |
There was a problem hiding this comment.
This seems to be polluting the API with testing types.
It also is trying to use a Test prefix here to prevent semantic API use. I would rather explore options that use the semantics of Go actually make the restriction by design.
There was a problem hiding this comment.
I propose this as an intermediate solution as noted in the description.
There was a problem hiding this comment.
I would rather we work on an permanent and intentional solution instead of a stop gap.
Towards #5192
While I do not think it is great I think it might be good enough for now as it can unblock exporter unit tests.
The benefit of this approach is that it is simple and someone who would want to alter record's scope or resource in a
Processor(which is not allowed by the specification) would need to explicitly use aTestRecordand even the name should resonate that it should not be used in production code.Maybe it would be better to have such functionality under
sdk/log/logtest. I believe that @MrAlias also mentioned this idea during the SIG meeting. I think we could do this by copyingrecord.goandrecord_test.gotologtestand addingSetResourceandSetInstrumentationScope(and tests) in seperate files (e.g.testrecord.goandtestrecord_test.go). It is possible to convert between types that have exactly same fields. See: https://go.dev/play/p/rMq8vtYDx5O. I can try making a PR like this on Tuesday.Still, maybe this PR is "good-enough" for now for unblocking the testing of exporter. But we can decide that it is not GA-ready. This is why I decided why this PR is "towards" fixing the issue (so that we are not closing yet). And we can try a better solution in a separate PR.