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: Same default environment #2447

Merged
merged 9 commits into from
Nov 28, 2022
Merged

Conversation

kevinrenskers
Copy link
Contributor

@kevinrenskers kevinrenskers commented Nov 25, 2022

📜 Description

SentryOptions is the source of truth for the default environment, and is used on events and sessions.

💡 Motivation and Context

Closes #2411

💚 How did you test it?

With a debug proxy to see the actual values getting sent, and with unit tests.

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • Review from the native team if needed
  • No breaking changes -> Do we classify this as a breaking change, or just a bug fix? 🤔

🔮 Next steps

@kevinrenskers
Copy link
Contributor Author

kevinrenskers commented Nov 25, 2022

Reproduction:

I first ran the sample app on the main branch, only removing the options.environment = "test-app" line, using a debug proxy (tip: Proxyman is great). I triggered an error and I can see that both the event and the session have the environment debug, as that's what's being set in the sample app (scope.setEnvironment("debug")).

I then also removed that line, and ran the sample app again, triggering an error. The event get environment "production" while the session gets no environment.


Why is the session's environment property publicly writable? This means that you can send different environment values for events and sessions, on purpose. Why?

@kevinrenskers kevinrenskers changed the base branch from master to 8.0.0 November 25, 2022 10:28
@github-actions
Copy link

github-actions bot commented Nov 25, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1256.72 ms 1275.39 ms 18.67 ms
Size 20.75 KiB 383.78 KiB 363.02 KiB

Baseline results on branch: 8.0.0

Startup times

Revision Plain With Sentry Diff
1ce879f 1258.12 ms 1260.90 ms 2.78 ms
f91714d 1222.06 ms 1247.00 ms 24.94 ms
7eee302 1228.73 ms 1241.94 ms 13.21 ms
dcac8ad 1238.82 ms 1247.80 ms 8.98 ms
2ae7db9 1231.37 ms 1239.98 ms 8.61 ms
d10145a 1232.65 ms 1257.55 ms 24.90 ms
68094b3 1214.14 ms 1255.09 ms 40.95 ms
e2cec76 1189.48 ms 1229.84 ms 40.36 ms
c9129b6 1231.86 ms 1270.11 ms 38.25 ms
bbe81cb 1257.25 ms 1272.24 ms 14.99 ms

App size

Revision Plain With Sentry Diff
1ce879f 20.75 KiB 381.81 KiB 361.06 KiB
f91714d 20.75 KiB 381.87 KiB 361.12 KiB
7eee302 20.75 KiB 374.73 KiB 353.97 KiB
dcac8ad 20.75 KiB 379.11 KiB 358.36 KiB
2ae7db9 20.75 KiB 381.87 KiB 361.12 KiB
d10145a 20.75 KiB 379.12 KiB 358.36 KiB
68094b3 20.75 KiB 373.94 KiB 353.19 KiB
e2cec76 20.75 KiB 381.81 KiB 361.06 KiB
c9129b6 20.75 KiB 381.81 KiB 361.06 KiB
bbe81cb 20.75 KiB 381.81 KiB 361.06 KiB

Previous results on branch: fix/2411-same-default-environment

Startup times

Revision Plain With Sentry Diff
326db6d 1231.00 ms 1252.20 ms 21.20 ms
6934b8c 1224.35 ms 1230.67 ms 6.33 ms
ebe9516 1229.90 ms 1237.77 ms 7.87 ms
5eb0059 1203.94 ms 1234.84 ms 30.90 ms
3e28f16 1218.06 ms 1237.36 ms 19.30 ms
968189e 1250.20 ms 1274.66 ms 24.46 ms
6552d08 1234.80 ms 1270.96 ms 36.16 ms

App size

Revision Plain With Sentry Diff
326db6d 20.75 KiB 383.78 KiB 363.03 KiB
6934b8c 20.75 KiB 383.78 KiB 363.03 KiB
ebe9516 20.75 KiB 383.78 KiB 363.03 KiB
5eb0059 20.75 KiB 383.74 KiB 362.99 KiB
3e28f16 20.75 KiB 383.78 KiB 363.03 KiB
968189e 20.75 KiB 383.75 KiB 362.99 KiB
6552d08 20.75 KiB 383.78 KiB 363.03 KiB

@@ -546,12 +545,6 @@ - (SentryEvent *_Nullable)prepareEvent:(SentryEvent *)event
event.dist = dist;
}

NSString *environment = self.options.environment;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why this bit code was here, to first set the event's environment, then do some other work, then set the event's environment again some lines later? It seems completely unnecessary at least, with the current changes where self.options.environment now always has a value.

@kevinrenskers kevinrenskers marked this pull request as ready for review November 25, 2022 12:53
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM

@brustolin
Copy link
Contributor

@philipphofmann do you know why SentrySession is public? I did not found a single public API that uses the session class.

@kevinrenskers kevinrenskers merged commit ff99713 into 8.0.0 Nov 28, 2022
@kevinrenskers kevinrenskers deleted the fix/2411-same-default-environment branch November 28, 2022 15:29
@philipphofmann
Copy link
Member

@philipphofmann do you know why SentrySession is public? I did not found a single public API that uses the session class.

I guess no good reason. I made it internal #2451.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sessions sent without an environment when environment option is not set
3 participants