mdatagen: get package name from metadata #11468
Conversation
I noticed a mistake in my previous PR open-telemetry#11232; some function calls did not pass the correct package name in (passing in "metadata" instead of the intended generated package name). This PR attempts to address the potential for this mistake to even occur by providing a wrapper `generateFile` function that automatically uses the generated package name from the metadata. The original version of the function that accepts a package name is intact for the templates that are going in the base package instead of the generated one.
braydonk
force-pushed
the
mdatagen_fix_package_name
branch
from
3cd7f36 to
a7da76a
Compare
|
I believe this change should not need a changelog. It is a fix for a bug that never made it into a release, and otherwise provides no user-visible changes. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11468 +/- ##
==========================================
- Coverage 91.43% 91.35% -0.09%
==========================================
Files 447 451 +4
Lines 23745 24275 +530
==========================================
+ Hits 21712 22176 +464
- Misses 1657 1717 +60
- Partials 376 382 +6 ☔ View full report in Codecov by Sentry. |
|
The new coverage issue are just for the generated |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Unstale, looks like this fell through the cracks before the original feature got merged. I'll introduce a changelog since this now fixes a released feature. |
| //go:generate mdatagen metadata.yaml | ||
| //go:generate mdatagen metadata_custom.yaml |
There was a problem hiding this comment.
Let's avoid the duplication. Maybe having a custom package in the processor and leaving the receiver as is.
There was a problem hiding this comment.
I opted to remove duplication slightly differently in f106762.
I left the custom package in samplereceiver. This still ensures the extent of coverage:
- Generates the representative set of templates for a metadata package
- Demonstrates the capability of generating two separate packages with different names
The custom package in sampleprocessor was not adding anything to the above coverage by just doing it in samplereceiver, so I removed that. I think the current duplication is acceptable since it still provides the necessary coverage, but if you disagree I can change it.
There was a problem hiding this comment.
I still don't understand why we need two packages for the same receiver. This will be confusing to maintain going forward. Also, we need to show how mdatagen is supposed to be used. Having two yaml files and two packages for the same module is not something we should recommend. Can we just add generated_package_name to the existing yaml for the receiver and move the package to a custom path?
There was a problem hiding this comment.
Having two yaml files and two packages is the reason the custom package name feature was added in the first place. It was the only reasonable way I could see for a receiver to have a featuregated semantic convention transition, as doing it within one metadata package seems infeasible based on my attempts. We wouldn't want two packages to exist forever, but for the lifetime of the featuregate (which for a semconv transition will likely be a longer horizon) two packages would need to coexist.
| scope_name: go.opentelemetry.io/collector/internal/receiver/samplereceiver | ||
| github_project: open-telemetry/opentelemetry-collector | ||
|
|
||
| generated_package_name: custom |
There was a problem hiding this comment.
internal/metadata package is abandoned now and should be removed, right?
There was a problem hiding this comment.
Commented too early under assumption that https://github.com/open-telemetry/opentelemetry-collector/pull/11468/files#diff-7d3195204f99934e80e03730709378a3054a1c056d953091368ad2232d282442R5 line is removed
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Not stale, still relevant for open-telemetry/opentelemetry-collector-contrib#35325 I'll wait until knowing the change isn't rejected to fix the conflict |
Description
I noticed a mistake in my previous PR #11232; some function calls did not pass the correct package name in (passing in "metadata" instead of the intended generated package name). This PR attempts to address the potential for this mistake to even occur by providing a wrapper
generateFilefunction that automatically uses the generated package name from the metadata. The original version of the function that accepts a package name is intact for the templates that are going in the base package instead of the generated one.Link to tracking issue
Bug originally from #11231
Fixes #11469
Testing
make mdatagen-testAlso added a custom generated package to the sample components, which would have caught the original bug.
Documentation