Skip to content

feat(flags): Add integration for custom tracking of flag evaluations#3860

Merged
aliu39 merged 24 commits into
masterfrom
aliu/ff-integration-and-added-coverage
Dec 19, 2024
Merged

feat(flags): Add integration for custom tracking of flag evaluations#3860
aliu39 merged 24 commits into
masterfrom
aliu/ff-integration-and-added-coverage

Conversation

@aliu39

@aliu39 aliu39 commented Dec 5, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Follow-up to

  1. Adds an integration for buffering feature flags manually with an API (add_feature_flag), and automatically capturing them on error events.

  2. Adds improved test coverage to launchdarkly and openfeature by testing end to end with capture_events fixture, rather than asserting the flags are stored to scope.

@aliu39
aliu39 requested a review from cmanallen December 5, 2024 19:57
@codecov

codecov Bot commented Dec 5, 2024 •
edited
Loading

Copy link
Copy Markdown

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
13801 1 13800 4139
View the top 1 failed tests by shortest run time 
tests.integrations.launchdarkly.test_launchdarkly test_launchdarkly_integration[False]
Stack Traces | 0.337s run time 
.../integrations/launchdarkly/test_launchdarkly.py:48: in test_launchdarkly_integration
    assert len(events) == 1
E   AssertionError: assert 2 == 1
E    +  where 2 = len([{'breadcrumbs': {'values': []}, 'contexts': {'runtime': {'build': '3.12.8 (main, Dec  4 2024, 06:20:10) [GCC 9.4.0]', 'name': 'CPython', 'version': '3.12.8'}, 'trace': {'parent_span_id': None, 'span_id': '91e2f801598b1cc5', 'trace_id': 'ec3ee1fd20f64251aee47ef1bfa3b656'}}, 'environment': 'production', 'event_id': '22cbdbda740f4333bc4a24d9477cd58f', ...}, {'breadcrumbs': {'values': []}, 'contexts': {'flags': {'values': [{'flag': 'hello', 'result': True}, {'flag': 'world', 'result': True}, {'flag': 'other', 'result': False}]}, 'runtime': {'build': '3.12.8 (main, Dec  4 2024, 06:20:10) [GCC 9.4.0]', 'name': 'CPython', 'version': '3.12.8'}, 'trace': {'parent_span_id': None, 'span_id': '91e2f801598b1cc5', 'trace_id': 'ec3ee1fd20f64251aee47ef1bfa3b656'}}, 'environment': 'production', 'event_id': 'b5b7269cd45740ad89573390823dbcbd', ...}])

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

cmanallen
cmanallen requested changes Dec 5, 2024
View reviewed changes
Comment thread sentry_sdk/integrations/featureflags.py
Comment thread tests/integrations/featureflags/test_featureflags.py Outdated
Comment thread tests/test_lru_cache.py
Comment thread sentry_sdk/_lru_cache.py
@aliu39 aliu39 changed the title Aliu/ff integration and added coverage feat(flags): Add integration for custom tracking of flag evaluations Dec 5, 2024
@aliu39
aliu39 force-pushed the aliu/ff-integration-and-added-coverage branch from 320e49e to 4651b6a Compare December 6, 2024 19:05
@aliu39
aliu39 requested a review from cmanallen December 6, 2024 19:31
@aliu39
aliu39 marked this pull request as ready for review December 6, 2024 19:31
@aliu39
aliu39 requested review from a team and antonpirker December 6, 2024 19:57
cmanallen
cmanallen requested changes Dec 6, 2024
View reviewed changes
Comment thread sentry_sdk/integrations/featureflags.py Outdated
Comment thread tests/integrations/featureflags/test_featureflags.py Outdated
@aliu39
aliu39 requested a review from cmanallen December 6, 2024 20:18
aliu39
aliu39 commented Dec 6, 2024
View reviewed changes
Comment thread tests/integrations/featureflags/test_featureflags.py Outdated


@pytest.fixture
def uninstall_integration():

@aliu39 aliu39 Dec 6, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Note on this fixture: I found using the reset_integrations fixture in conftest.py caused issues with the default dedupe integration. We just want the integration being tested to be setup at the start of every new test.

Without using this, the error processor isn't added correctly. This isn't a concern for real use, since user apps only call init once

@aliu39
aliu39 requested a review from a team December 10, 2024 21:36
@billyvg billyvg mentioned this pull request Dec 11, 2024
Closed
@aliu39 aliu39 mentioned this pull request Dec 16, 2024
Closed
antonpirker
antonpirker approved these changes Dec 18, 2024
View reviewed changes

@antonpirker antonpirker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks OK

@antonpirker

antonpirker commented Dec 18, 2024

Copy link
Copy Markdown
Contributor

We also need to add docs for this, can you prepare a PR in the docs repo?

@aliu39

aliu39 commented Dec 18, 2024

Copy link
Copy Markdown
Member Author

Sounds good! I've opened getsentry/sentry-docs#12152, will do a last check in the morning to make sure it's up to date!

@aliu39

aliu39 commented Dec 18, 2024

Copy link
Copy Markdown
Member Author

Do you know anything about the failing ubuntu tests @antonpirker ?

@antonpirker

antonpirker commented Dec 18, 2024

Copy link
Copy Markdown
Contributor

Do you know anything about the failing ubuntu tests @antonpirker ?

In Python 3.6 there is probably no asyncio.run().

Can you fix this somehow, or skip this test in Python 3.6 (then this integration will only be supported form Python 3.7 and up)

@aliu39
aliu39 enabled auto-merge (squash) December 19, 2024 04:00
@aliu39
aliu39 disabled auto-merge December 19, 2024 04:00
@aliu39
aliu39 enabled auto-merge (squash) December 19, 2024 04:01
@aliu39
aliu39 merged commit 50222ca into master Dec 19, 2024
@aliu39
aliu39 deleted the aliu/ff-integration-and-added-coverage branch December 19, 2024 04:33
@aliu39 aliu39 mentioned this pull request Jan 6, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@billyvg billyvg billyvg left review comments
@cmanallen cmanallen cmanallen approved these changes
+1 more reviewer
@antonpirker antonpirker antonpirker approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@aliu39 @antonpirker @billyvg @cmanallen