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

Sentry: add more env context on errors #386

Merged
merged 1 commit into from
Jul 7, 2022
Merged

Conversation

gre
Copy link
Contributor

@gre gre commented Jun 17, 2022

📝 Description

On Sentry, we currently don't know if any experimental OR feature flag has been enabled in context of an error, which is very valuable to know about.

Trade-off: this works with "polling". The current implementation will regularly sync the tags so they are always correct when an error arise.

This PR only do this on LLM as the LLD PR isn't merged there, but we'll eventually also apply the same idea to LLD.

❓ Context

  • Impacted projects: LLM and LLD

✅ Checklist

📸 Demo

We can see the env and the feature flags

Screenshot 2022-07-04 at 19 15 01

🚀 Expectations to reach

Please make sure you follow these Important Steps.

Pull Requests must pass the CI and be internally validated in order to be merged.

@vercel
Copy link

vercel bot commented Jun 17, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
live-common-tools ✅ Ready (Inspect) Visit Preview Jul 5, 2022 at 3:34PM (UTC)
3 Ignored Deployments
Name Status Preview Updated
ledger-live-github-bot ⬜️ Ignored (Inspect) Jul 5, 2022 at 3:34PM (UTC)
native-ui-storybook ⬜️ Ignored (Inspect) Jul 5, 2022 at 3:34PM (UTC)
react-ui-storybook ⬜️ Ignored (Inspect) Jul 5, 2022 at 3:34PM (UTC)

@changeset-bot
Copy link

changeset-bot bot commented Jun 17, 2022

🦋 Changeset detected

Latest commit: 9062bdc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
ledger-live-desktop Patch
live-mobile Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link

github-actions bot commented Jun 17, 2022

@gre

Screenshots: ✅

There are no changes in the screenshots for this PR. If this is expected, you are good to go.

@gre gre force-pushed the support/sentry-extra-tags branch from 562f369 to 3b2a936 Compare June 20, 2022 16:58
@github-actions github-actions bot added the desktop Has changes in LLD label Jun 20, 2022
@gre gre force-pushed the support/sentry-extra-tags branch from 3b2a936 to e03eea9 Compare June 20, 2022 17:08
@gre gre changed the title LLM Sentry: add more env context on errors Sentry: add more env context on errors Jun 24, 2022
@gre gre force-pushed the support/sentry-extra-tags branch from e03eea9 to 28cc23a Compare July 4, 2022 15:20
@codecov
Copy link

codecov bot commented Jul 4, 2022

Codecov Report

Merging #386 (9062bdc) into develop (cf80564) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #386   +/-   ##
========================================
  Coverage    47.98%   47.98%           
========================================
  Files          607      607           
  Lines        26954    26954           
  Branches      6897     6897           
========================================
  Hits         12935    12935           
  Misses       13959    13959           
  Partials        60       60           
Flag Coverage Δ
test 47.98% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf80564...9062bdc. Read the comment docs.

@gre gre force-pushed the support/sentry-extra-tags branch from 28cc23a to ff48eae Compare July 4, 2022 17:05
@gre gre marked this pull request as ready for review July 4, 2022 17:15
Copy link
Contributor

@ofreyssinet-ledger ofreyssinet-ledger left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@gre gre merged commit 8917ca1 into develop Jul 7, 2022
@gre gre deleted the support/sentry-extra-tags branch July 7, 2022 14:57
// We need to wait firebase to load the data and then we set once for all the tags
setTimeout(syncTheTags, 5000);
// We also try to regularly update them so we are sure to get the correct tags (as these are dynamic)
setInterval(syncTheTags, 60000);
Copy link
Contributor

@elbywan elbywan Jul 7, 2022

Choose a reason for hiding this comment

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

I understand that this component is high up the tree, and will likely not re-render often but if for some reason it does we really need to return a cleanup function that will clear the interval (and the timeout, even though it's less critical).

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -24,11 +24,14 @@ import * as Sentry from "@sentry/react-native";
import Config from "react-native-config";
import VersionNumber from "react-native-version-number";

import { getEnv } from "@ledgerhq/live-common/lib/env";
Copy link
Contributor

Choose a reason for hiding this comment

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

since the common module is now transpiled to esm it would be better to not rely on the commonjs bundle @ledgerhq/live-common/lib/env => @ledgerhq/live-common/env

gre added a commit that referenced this pull request Jul 7, 2022
@gre gre mentioned this pull request Jul 7, 2022
3 tasks
gre added a commit that referenced this pull request Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop Has changes in LLD mobile Has changes in LLM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants