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

Jump to bottom

bug 1847950 - Trigger upload after scanning pending pings #2621

Merged
merged 1 commit into from
Oct 18, 2023
Merged

bug 1847950 - Trigger upload after scanning pending pings #2621

merged 1 commit into from
Oct 18, 2023

Conversation

chutten
Copy link
Contributor

@chutten chutten commented Sep 15, 2023

No description provided.

@chutten chutten requested a review from a team as a code owner September 15, 2023 20:14
@chutten chutten requested review from rosahbruno and removed request for a team September 15, 2023 20:14
@chutten chutten force-pushed the bug1847950-triggerUploadAfterPendingPingScan branch 3 times, most recently from aeaf94c to 9eab439 Compare September 18, 2023 20:49
@chutten chutten requested a review from badboy September 21, 2023 13:28
@chutten
Copy link
Contributor Author

chutten commented Sep 21, 2023

This is the one where we need to give a think to how and when we should use the global STATE inside the SDK. Should global_state() be fallible? Should the Glean instance have references to some pieces of the state?

+Cc @badboy who was going to give this a noodle.

@badboy
Copy link
Member

badboy commented Sep 21, 2023

This is the one where we need to give a think to how and when we should use the global STATE inside the SDK. Should global_state() be fallible? Should the Glean instance have references to some pieces of the state?

+Cc @badboy who was going to give this a noodle.

Right right. And I kinda did a bit and nothing satisfying came out of this.
As said in channel we're essentially crossing abstraction boundaries here, but there's also no good way to handle it better without re-designing a lot of this.
We could provide a fallible version of it and use that there, fully aware that it's imperfect. If the Kotlin and Swift tests pass then I'm more confident it works. But writing an appropriate test is also hard (because ... ugh ... this is all kinda async and stuff).

@chutten
Copy link
Contributor Author

chutten commented Sep 21, 2023

This is the one where we need to give a think to how and when we should use the global STATE inside the SDK. Should global_state() be fallible? Should the Glean instance have references to some pieces of the state?
+Cc @badboy who was going to give this a noodle.

Right right. And I kinda did a bit and nothing satisfying came out of this. As said in channel we're essentially crossing abstraction boundaries here, but there's also no good way to handle it better without re-designing a lot of this. We could provide a fallible version of it and use that there, fully aware that it's imperfect. If the Kotlin and Swift tests pass then I'm more confident it works. But writing an appropriate test is also hard (because ... ugh ... this is all kinda async and stuff).

Well, the threadcrash test covers this fairly decently, by proving that there is an additional thread spawned when we expect it to be spawned. Would that do?

@chutten chutten force-pushed the bug1847950-triggerUploadAfterPendingPingScan branch 3 times, most recently from 3fe7b72 to 2d2addc Compare September 25, 2023 20:10
@chutten chutten force-pushed the bug1847950-triggerUploadAfterPendingPingScan branch from 2d2addc to 7e394ef Compare October 17, 2023 18:38
@firefoxci-taskcluster
Copy link

Uh oh! Looks like an error! Details

Taskcluster-GitHub attempted to cancel previously created task groups with following scopes:

assume:repo:github.com/mozilla/glean:*, queue:seal-task-group:taskcluster-github/*, queue:cancel-task-group:taskcluster-github/*

Client ID static/taskcluster/github does not have sufficient scopes and is missing the following scopes:

queue:seal-task-group:glean-level-1/ecuikb8FRxKo7fq9GcFCdg

This request requires the client to satisfy the following scope expression:

queue:seal-task-group:glean-level-1/ecuikb8FRxKo7fq9GcFCdg
  • method: sealTaskGroup
  • errorCode: InsufficientScopes
  • statusCode: 403
  • time: 2023-10-17T18:38:11.772Z

@chutten
bug 1847950 - Trigger upload after scanning pending pings
ed1bbea 
This requires changing the thread crashing test as we now launch another
uploader thread that needs to be crashed.

Continues the questionable trend of calling Global STATE Callbacks from inside
glean-core specifically to trigger upload if possible.

Uses the dispatcher to avoid deadlock (see bug 1859574).
@chutten chutten force-pushed the bug1847950-triggerUploadAfterPendingPingScan branch from 7e394ef to ed1bbea Compare October 17, 2023 19:26
@firefoxci-taskcluster
Copy link

Uh oh! Looks like an error! Details

Taskcluster-GitHub attempted to cancel previously created task groups with following scopes:

assume:repo:github.com/mozilla/glean:*, queue:seal-task-group:taskcluster-github/*, queue:cancel-task-group:taskcluster-github/*

Client ID static/taskcluster/github does not have sufficient scopes and is missing the following scopes:

queue:seal-task-group:glean-level-1/NZqfThMLSYK8ylvFk5xw2g

This request requires the client to satisfy the following scope expression:

queue:seal-task-group:glean-level-1/NZqfThMLSYK8ylvFk5xw2g
  • method: sealTaskGroup
  • errorCode: InsufficientScopes
  • statusCode: 403
  • time: 2023-10-17T19:26:34.525Z

@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (2fcf5ad) 33.33% compared to head (ed1bbea) 33.33%.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #2621   +/-   ##
=======================================
  Coverage   33.33%   33.33%           
=======================================
  Files           1        1           
  Lines          42       42           
=======================================
  Hits           14       14           
  Misses         28       28

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

badboy
badboy approved these changes Oct 18, 2023
View reviewed changes
Copy link
Member

@badboy badboy left a comment

Choose a reason for hiding this comment

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

triggered iOS CI. Code looks good.

glean-core/android/src/test/java/mozilla/telemetry/glean/pings/CustomPingTest.kt Show resolved Hide resolved
@chutten chutten merged commit c91af19 into mozilla:main Oct 18, 2023
7 checks passed
@chutten chutten deleted the bug1847950-triggerUploadAfterPendingPingScan branch October 18, 2023 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@badboy badboy badboy approved these changes

@rosahbruno rosahbruno Awaiting requested review from rosahbruno rosahbruno is a code owner automatically assigned from mozilla/glean

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

Successfully merging this pull request may close these issues.
2 participants
@chutten @badboy