fix: use (name, subtype) tuple as deduplication key in add_deprecated_components#522
Closed
Rama542 wants to merge 1 commit into
Closed
Conversation
…_components The add_deprecated_components method was checking for duplicates using component name alone. This caused it to silently skip a deprecation entry when a component with the same name already existed in the index under a different subtype. For example, if zipkin encoding was already recorded as deprecated and zipkin storage was later removed, the storage entry would be dropped because zipkin was already seen in existing_names. Changed the deduplication set to track (name, subtype) tuples so each combination is treated as a distinct entry. Added two new test cases: one that confirms both subtype entries are written when they differ, and one that confirms a true duplicate (same name and same subtype) is still skipped correctly. This is a companion fix to the same bug addressed in DeprecationDetector where _build_component_set was also keying on name alone. Closes open-telemetry#519
✅ Deploy Preview for otel-ecosystem-explorer canceled.
|
Member
|
is this a duplicate of #498 ? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #523
What this fixes
The
add_deprecated_componentsmethod inInventoryManagerwas checking for duplicates using only the component name. That works fine when every component name is unique, but it breaks down as soon as two components share the same name under different subtypes.Here is a concrete example. The Collector has extensions under both
extension/encodingandextension/storage. If a component calledzipkinexisted under both, and the encoding variant was removed first and recorded as deprecated, then when the storage variant was removed later, the code would findzipkinalready inexisting_namesand skip the second entry entirely. The storage deprecation would be permanently lost from the registry with no warning.The fix
Changed the deduplication set from a flat set of names to a set of
(name, subtype)tuples. This is exactly the same approach used to fixDeprecationDetector._build_component_setin PR 498 and issue 493. The detection side was already corrected there, and this PR corrects the persistence side so correctly detected removals are not dropped before they reach the registry.Before:
After:
Tests
Added two new test cases in
test_inventory.py:test_add_deprecated_components_same_name_different_subtypeVerifies that when
zipkin (encoding)is already in the index andzipkin (storage)is newly deprecated, both entries end up in the index.test_add_deprecated_components_no_duplicate_same_name_and_subtypeVerifies that an entry with the exact same name and the exact same subtype is still correctly skipped, so the existing deduplication behavior is preserved.
All 145 tests in the collector-watcher suite pass.