Add OpenTelemetry instrumentation to IPFS retrieval#7
Open
0xSwego wants to merge 4 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds OpenTelemetry tracing and metrics around IPFS Zarr dataset loading: OTEL setup, metric/span helpers, CID validation, a span-wrapped loader that records per-store and dataset metrics and maps connection-like failures, plus tests for error classification and timeout behavior. ChangesOTEL Instrumentation for IPFS Dataset Retrieval
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dclimate_client_py/ipfs_retrieval.py`:
- Around line 225-304: The sharded-store except block (the catch "except
Exception as sharded_err" around ShardedZarrStore) must classify connection
failures the same way you do later and raise IpfsConnectionError before
attempting the HAMT fallback; inspect sharded_err for the same
substrings/conditions used to create IpfsConnectionError (e.g., "Connection
refused", "Max retries exceeded", "Timeout") and if matched call
_record_span_error(dataset_span, connection_error) and raise that
connection_error instead of falling back to HAMT; otherwise proceed with the
existing HAMT.build/ZarrHAMTStore fallback, recording metrics via
_record_store_open and setting ds.attrs["_ipfs_store_type"], dataset_status and
dataset_store_type as currently done.
- Around line 183-206: The spans started with _TRACER.start_as_current_span(...)
(e.g., the ShardedZarrStore/HAMT/dataset spans that call _record_span_error in
their except blocks) are currently being double-reported because you both
manually call _record_span_error(span, err) and let the context manager
auto-record exceptions; fix by disabling automatic exception recording on those
spans—call start_as_current_span with record_exception=False and
set_status_on_exception=False for the spans used around ShardedZarrStore.open,
the HAMT open, and the dataset span (where _record_span_error is invoked), so
the manual _record_span_error remains the sole reporter of the exception and
status.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61dfdaea-2424-4dcc-95ce-9ea11e5b23c2
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
dclimate_client_py/ipfs_retrieval.pypyproject.toml
0xSwego
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_ipfs_retrieval.py (1)
14-16: 💤 Low valueConsider clarifying or removing the no-op fixture.
The
check_ipfs_connectionfixture name suggests it validates IPFS availability, but it currently does nothing. Since the tests in this file use mocks and don't require a real IPFS connection, you might:
- Remove the fixture if it's not needed, or
- Add a docstring explaining it's a placeholder for future IPFS-dependent tests, or
- Implement actual IPFS availability checking if tests will later need it
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_ipfs_retrieval.py` around lines 14 - 16, The fixture check_ipfs_connection is a no-op that misleads readers; either remove the fixture entirely from tests/test_ipfs_retrieval.py if no real IPFS connectivity is required, or replace it with a clear placeholder docstring explaining it's intentionally a stub for future IPFS-dependent tests, or implement an actual availability check (e.g., attempt a quick ping/HTTP request to the IPFS API and skip tests on failure) so that the fixture name matches behavior; locate the check_ipfs_connection fixture in the file and apply one of these three options.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_ipfs_retrieval.py`:
- Around line 14-16: The fixture check_ipfs_connection is a no-op that misleads
readers; either remove the fixture entirely from tests/test_ipfs_retrieval.py if
no real IPFS connectivity is required, or replace it with a clear placeholder
docstring explaining it's intentionally a stub for future IPFS-dependent tests,
or implement an actual availability check (e.g., attempt a quick ping/HTTP
request to the IPFS API and skip tests on failure) so that the fixture name
matches behavior; locate the check_ipfs_connection fixture in the file and apply
one of these three options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 272c060f-723a-4e4d-9cf6-8f9b3b39368d
📒 Files selected for processing (4)
dclimate_client_py/ipfs_retrieval.pytests/test_ipfs_retrieval.pytests/test_list_datasets_parity.pytests/test_stac_server_listing.py
✅ Files skipped from review due to trivial changes (2)
- tests/test_list_datasets_parity.py
- tests/test_stac_server_listing.py
🚧 Files skipped from review as they are similar to previous changes (1)
- dclimate_client_py/ipfs_retrieval.py
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
1 participant
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.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores