Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix clickhouse-driver integration spans #3638

Merged
merged 10 commits into from
Oct 10, 2024

Conversation

sentrivana
Copy link
Contributor

@sentrivana sentrivana commented Oct 9, 2024

General

  • change POTelSpan._get_attribute to public POTelSpan.get_attribute

clickhouse-driver integration

  • Change set_data to set_attribute
  • Span attr db.params is now a string
  • params are now stored on the connection temporarily. Previously, they were saved on the span, but since we're setting them once and then reading them and appending to them in another place, span attrs no longer work for this nicely -- we'd need to serialize them to str, then deserialize to add stuff, and serialize again. So I'm changing this to keep track of them elsewhere to avoid this dance.
  • Span attr db.query.text has been added. Previously, we were storing query on the span temporarily, but then were getting rid of it. This PR preserves it and gives it an OTel-friendly name. (This also means the query text is now stored on the span 3x -- in the description, in sentry.name and in db.query.text.)
  • We were checking same_process_as_parent in the tests, now we're not.
  • In OTel, the spans now have a status attribute, we're also not checking this in the tests.

Caveats

Copy link

codecov bot commented Oct 9, 2024

❌ 2933 Tests Failed:

Tests completed Failed Passed Skipped
20072 2933 17139 4443
View the top 3 failed tests by shortest run time
tests.integrations.litestar.test_litestar test_middleware_spans
Stack Traces | 0s run time
No failure message available
tests.integrations.celery.test_update_celery_task_headers test_monitor_beat_tasks[False]
Stack Traces | 0.001s run time
.../integrations/celery/test_update_celery_task_headers.py:38: in test_monitor_beat_tasks
    assert "sentry-monitor-start-timestamp-s" not in outgoing_headers["headers"]
E   KeyError: 'headers'
tests.test_scope test_with_isolation_scope_data
Stack Traces | 0.001s run time
tests/test_scope.py:387: in test_with_isolation_scope_data
    assert scope._tags == {"before_isolation_scope": 1}
E   AssertionError: assert {} == {'before_isolation_scope': 1}
E     Right contains 1 more item:
E     {'before_isolation_scope': 1}
E     Full diff:
E     - {'before_isolation_scope': 1}
E     + {}

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

@@ -91,7 +91,7 @@ def _inner(*args: P.args, **kwargs: P.kwargs) -> T:

_set_db_data(span, connection)

span.set_data("query", query)
span.set_data("db.query.text", query)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

span.scope.add_breadcrumb(
message=span._data.pop("query"), category="query", data=span._data
)
query = span._get_attribute("db.query.text")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we need to access attributes outside, let's officially expose get_attributes or a @property called attributes. Accessing underscore methods outside the implementation is an anti-pattern and we should stop doing that.

@sentrivana sentrivana changed the title Fix data in clickhouse-driver integration Fix clickhouse-driver integration spans Oct 10, 2024
@sentrivana sentrivana marked this pull request as ready for review October 10, 2024 08:40
@sentrivana sentrivana merged commit a67125a into potel-base Oct 10, 2024
15 of 125 checks passed
@sentrivana sentrivana deleted the ivana/potel/fix-clickhouse branch October 10, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants