Skip to content

fix(django): add check for instrument view mro #1744

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Oct 23, 2020
Merged

fix(django): add check for instrument view mro #1744

merged 13 commits into from
Oct 23, 2020

Conversation

majorgreys
Copy link
Contributor

@majorgreys majorgreys commented Oct 22, 2020 •
edited by Kyle-Verhoog
Loading

Description

We had been silently failing to instrument views for feed-view, partial-view and include because of a change introduced with #1625. Tracer debug logs do show this. This PR fixes the instrumentation code to check if the view has an mro before traversing it and fixes the tests by adding snapshot regressions.

Checklist

  • Entry added to release notes using reno
  • Tests provided; and/or
  • Description of manual testing performed and explanation is included in the code and/or PR.
  • Library documentation is updated.
  • Corp site documentation is updated (link to the PR).

@majorgreys majorgreys requested a review from a team as a code owner October 22, 2020 18:22
@Kyle-Verhoog Kyle-Verhoog added the changelog/no-changelog A changelog entry is not required for this PR. label Oct 22, 2020
@Kyle-Verhoog Kyle-Verhoog force-pushed the majorgreys/django-skip-view branch from b949596 to 4269e8f Compare October 22, 2020 21:24
Kyle-Verhoog
Kyle-Verhoog previously approved these changes Oct 22, 2020
View reviewed changes
...napshots/tests.contrib.django.test_django_snapshots.test_middleware_trace_callable_view.snap
"parent_id" 22
"start" 1603387537985202934
"duration" 291333}
{"name" "django.view"
Copy link
Member

Choose a reason for hiding this comment

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

👍 this span was not being generated before.

...ots/tests.contrib.django.test_django_snapshots.test_middleware_trace_callable_view_111x.snap
"parent_id" 0
"start" 1603387527594692000
"duration" 18000}
{"name" "django.view"
Copy link
Member

Choose a reason for hiding this comment

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

👍

...hots/tests.contrib.django.test_django_snapshots.test_middleware_trace_callable_view_18x.snap
"parent_id" 0
"start" 1603387459871710000
"duration" 311000}
{"name" "django.view"
Copy link
Member

Choose a reason for hiding this comment

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

👍

...ots/tests.contrib.django.test_django_snapshots.test_middleware_trace_partial_based_view.snap
"parent_id" 22
"start" 1603387538032015035
"duration" 6002}
{"name" "django.view"
Copy link
Member

Choose a reason for hiding this comment

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

👍

...ests.contrib.django.test_django_snapshots.test_middleware_trace_partial_based_view_111x.snap
"parent_id" 0
"start" 1603387527648070000
"duration" 17000}
{"name" "django.view"
Copy link
Member

Choose a reason for hiding this comment

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

👍

...tests.contrib.django.test_django_snapshots.test_middleware_trace_partial_based_view_18x.snap
"parent_id" 0
"start" 1603387459925138000
"duration" 258000}
{"name" "django.view"
Copy link
Member

Choose a reason for hiding this comment

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

👍

...tests.contrib.django.test_django_snapshots.test_middleware_trace_partial_based_view_21x.snap
"parent_id" 22
"start" 1603387534004599101
"duration" 11020}
{"name" "django.view"
Copy link
Member

Choose a reason for hiding this comment

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

👍

...tests.contrib.django.test_django_snapshots.test_middleware_trace_partial_based_view_21x.snap
"duration" 11020}
{"name" "django.view"
"service" "django"
"resource" "partial"
Copy link
Member

Choose a reason for hiding this comment

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

This resource name isn't particularly useful. We should probably follow up addressing this.

tests/snapshots/tests.contrib.django.test_django_snapshots.test_urlpatterns_include.snap
"parent_id" 22
"start" 1603387537920451875
"duration" 4969}
{"name" "django.view"
Copy link
Member

Choose a reason for hiding this comment

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

👍

tests/snapshots/tests.contrib.django.test_django_snapshots.test_urlpatterns_include_21x.snap
"parent_id" 22
"start" 1603387533906915258
"duration" 7068}
{"name" "django.view"
Copy link
Member

Choose a reason for hiding this comment

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

👍

@Kyle-Verhoog Kyle-Verhoog removed the changelog/no-changelog A changelog entry is not required for this PR. label Oct 22, 2020
@Kyle-Verhoog Kyle-Verhoog merged commit 5ce4331 into master Oct 23, 2020
@Kyle-Verhoog Kyle-Verhoog deleted the majorgreys/django-skip-view branch October 23, 2020 00:41
Czechh added a commit that referenced this pull request Oct 23, 2020
@Czechh
Merge branch 'master' into sergio.prada/inject-trace-data-in-lambda-i…
120feb6 
…nvocations

* master:
  ci: introduce riot, use it to run tracer tests (#1737)
  fix(django): add check for instrument view mro (#1744)
  fix: snapshot decorator for pytest fixtures (#1743)
  tests(logging): don't test warn (#1738)
  feat(flask): store request headers (#1735)
  ci: fix deploy_master (#1739)
@majorgreys majorgreys mentioned this pull request Oct 30, 2020
Merged
5 tasks
@Kyle-Verhoog Kyle-Verhoog added this to the 0.44.0 milestone Nov 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kyle-Verhoog Kyle-Verhoog Kyle-Verhoog approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.44.0
Development

Successfully merging this pull request may close these issues.
2 participants
@majorgreys @Kyle-Verhoog