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.

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

Hubs/Scopes Merge 27 - Discussions #3352

Merged
merged 33 commits into from
Apr 22, 2024
Merged

Hubs/Scopes Merge 27 - Discussions #3352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
305baf5
replace hub with scopes
adinauer Mar 27, 2024
95f5e1b
Add Scopes
adinauer Apr 2, 2024
27f2398
Introduce `IScopes` interface.
adinauer Apr 2, 2024
ce3c14f
Replace `IHub` with `IScopes` in core
adinauer Apr 2, 2024
ce615f4
Replace `IHub` with `IScopes` in android core
adinauer Apr 2, 2024
22ddc00
Replace `IHub` with `IScopes` in android integrations
adinauer Apr 2, 2024
305c217
Replace `IHub` with `IScopes` in apollo integrations
adinauer Apr 2, 2024
da927bc
Replace `IHub` with `IScopes` in okhttp integration
adinauer Apr 2, 2024
8279276
Replace `IHub` with `IScopes` in graphql integration
adinauer Apr 2, 2024
9bfc086
Replace `IHub` with `IScopes` in logging integrations
adinauer Apr 2, 2024
b998e50
Replace `IHub` with `IScopes` in more integrations
adinauer Apr 2, 2024
739827a
Replace `IHub` with `IScopes` in OTel integration
adinauer Apr 2, 2024
69f2d63
Replace `IHub` with `IScopes` in Spring 5 / Spring Boot 2 integrations
adinauer Apr 2, 2024
792d482
Replace `IHub` with `IScopes` in Spring 6 / Spring Boot 3 integrations
adinauer Apr 2, 2024
9bcbce6
Replace `IHub` with `IScopes` in samples
adinauer Apr 2, 2024
3f25a4b
Merge branch 'feat/hsm-13-replacements-in-samples' into feat/hubs-sco…
adinauer Apr 2, 2024
d6fb40a
gitscopes -> github
adinauer Apr 2, 2024
7752bcc
Replace ThreadLocal with ScopesStorage
adinauer Apr 4, 2024
1e329c5
Move client and throwable to span map to scope
adinauer Apr 4, 2024
b0d89ae
Add global scope
adinauer Apr 4, 2024
cdd414a
use global scope in Scopes
adinauer Apr 4, 2024
98da9ff
Implement pushScope popScope and withScope for Scopes
adinauer Apr 4, 2024
2d26033
Add pushIsolationScope; add fork methods to ISCope
adinauer Apr 12, 2024
bbb6700
Use separate scopes for current, isolation and global scope; rename m…
adinauer Apr 12, 2024
c714b21
Allow controlling which scope configureScope uses
adinauer Apr 12, 2024
a474402
Combine scopes
adinauer Apr 12, 2024
ae93e33
Use new API for CRONS integrations
adinauer Apr 12, 2024
b01298b
Add lifecycle helper
adinauer Apr 12, 2024
b64e688
Change spring integrations to use new API
adinauer Apr 12, 2024
d06fc50
Use new API in servlet integrations
adinauer Apr 12, 2024
f0af5c3
Use new API for kotlin coroutines and wrapers for Supplier/Callable
adinauer Apr 12, 2024
2f02001
Discussion TODOs
adinauer Apr 12, 2024
2b05019
Merge branch '8.x.x' into feat/hsm-27-discussions
adinauer Apr 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ allprojects {
dependsOn("cleanTest")
}
withType<JavaCompile> {
options.compilerArgs.addAll(arrayOf("-Xlint:all", "-Werror", "-Xlint:-classfile", "-Xlint:-processing"))
options.compilerArgs.addAll(arrayOf("-Xlint:all", "-Werror", "-Xlint:-classfile", "-Xlint:-processing", "-Xlint:-try"))
Copy link
Member Author

Choose a reason for hiding this comment

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

This allows us to write:

try(ISentryLifecycleToken ignored = scopes.makeCurrent()) {
	...
}

and rely on AutoClosable to clean up scopes.

Copy link
Member

Choose a reason for hiding this comment

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

👍 why do you have to disable this warning? Does it complain every time you're not using the API with a try block?

Copy link
Member Author

Choose a reason for hiding this comment

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

It complains about not using the variable (called ignored here) inside the try {} block.

}
}
}
Expand Down
179 changes: 165 additions & 14 deletions sentry/api/sentry.api
View file
Open in desktop

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions sentry/src/main/java/io/sentry/Breadcrumb.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,6 +663,7 @@ public void setUnknown(@Nullable Map<String, Object> unknown) {
@Override
@SuppressWarnings("JavaUtilDate")
public int compareTo(@NotNull Breadcrumb o) {
// TODO also use nano time if equal
Copy link
Member Author

@adinauer adinauer Apr 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

To fix breadcrumb ordering across multiple collections when combining scopes - so we can maintain insertion order as well as possible. If this isn't good enough we might need some atomic counter we increment for each breadcrumb.

Copy link
Member Author

Choose a reason for hiding this comment

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

Implemented in #3355

return timestamp.compareTo(o.timestamp);
}

Expand Down
4 changes: 4 additions & 0 deletions sentry/src/main/java/io/sentry/CombinedScopeView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,7 @@ public void setTag(@NotNull String key, @NotNull String value) {

@Override
public void removeTag(@NotNull String key) {
// TODO should this go to all scopes?
Copy link
Member Author

Choose a reason for hiding this comment

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

Deleting certain things from scope can be tricky with new API since deletions might go to the wrong scope. Maybe we should just delete from all scopes. On the other hand a user might just want to shadow something temporarily so maybe deleting everywhere isn't wanted.

Copy link
Member Author

Choose a reason for hiding this comment

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

When calling removeX on current scope where scope has been forked since adding e.g. the tag in a parent scope, the remove only removes it from the current scope, not from the parent.

Copy link
Member

Choose a reason for hiding this comment

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

Good point, yeah that's tricky!
From a user perspective, I expect it to behave in-sync with setting a value, so I wouldn't remove the value from all scopes.
I'm wondering if we could treat remove<xy>() like a set<xy>(null) instead.

When retrieving scope values we then would need to change the behavior as well. From ~

return current.value ?: scope.value ?: global.value

To something like this ~

return if (current.isSet) {
	current.value // can still be null
} else if (scope.isSet) {
	scope.value
} else {
	global.value
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I agree on not removing from all and making it behave the same way set does.

Not so sure about setting null and returning that because how do you ever unset this?

As a workaround for users actually wanting to clear certain values from all scopes, we could offer a ScopeType.ALL where we send set, remove etc. to all scopes, when they run Sentry.configureScope(ScopeType.ALL) { scope -> ... }

Copy link
Member Author

Choose a reason for hiding this comment

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

If users want to shadow something in the sense of temporarily unsetting, we'd have to use Optional for all values I guess.

getDefaultWriteScope().removeTag(key);
}

Expand All @@ -256,6 +257,7 @@ public void setExtra(@NotNull String key, @NotNull String value) {

@Override
public void removeExtra(@NotNull String key) {
// TODO should this go to all scopes?
getDefaultWriteScope().removeExtra(key);
}

Expand Down Expand Up @@ -305,10 +307,12 @@ public void setContexts(@NotNull String key, @NotNull Character value) {

@Override
public void removeContexts(@NotNull String key) {
// TODO should this go to all scopes?
getDefaultWriteScope().removeContexts(key);
}

private @NotNull IScope getDefaultWriteScope() {
// TODO use Scopes.getSpecificScope?
Copy link
Member Author

Choose a reason for hiding this comment

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

Implemented in #3361

if (ScopeType.CURRENT.equals(getOptions().getDefaultScopeType())) {
return scope;
}
Expand Down
5 changes: 4 additions & 1 deletion sentry/src/main/java/io/sentry/ScopeType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,8 @@
public enum ScopeType {
CURRENT,
ISOLATION,
GLOBAL;
GLOBAL,

// TODO do we need a combined as well so configureScope
COMBINED;
Copy link
Member Author

Choose a reason for hiding this comment

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

This would allow users to use .configureScope() and be able to access the combined view of all scopes relevant, so they can read the same way SentryClient would.

Copy link
Member

Choose a reason for hiding this comment

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

The use case would be relevant for reading data from the scope right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, cross platform SDKs (via InternalSentrySdk) are the primary use case atm. I'm about to play around with this but I'm gonna need help testing cross platform SDKs after the change.

}
2 changes: 2 additions & 0 deletions sentry/src/main/java/io/sentry/Scopes.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,7 @@ public void endSession() {
}

private IScope getCombinedScopeView() {
// TODO create in ctor?
Copy link
Member Author

Choose a reason for hiding this comment

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

Implemented in #3361

return new CombinedScopeView(getGlobalScope(), isolationScope, scope);
}

Expand Down Expand Up @@ -428,6 +429,7 @@ public void addBreadcrumb(final @NotNull Breadcrumb breadcrumb, final @Nullable
}

private IScope getSpecificScope(final @Nullable ScopeType scopeType) {
// TODO extract and reuse
Copy link
Member Author

Choose a reason for hiding this comment

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

move to CombinedScopeView as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Implemented in #3361

if (scopeType != null) {
switch (scopeType) {
case CURRENT:
Expand Down
Loading