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

[Access] Unify Event streaming endpoint on the execution data gRPC API #5602

Merged
merged 34 commits into from
Apr 4, 2024

Conversation

UlyanaAndrukhiv
Copy link
Contributor

#5557

Context

In this PR :

  • implemented separated gRPC calls for event streaming:

    • SubscribeEventsFromStartBlockID,
    • SubscribeEventsFromStartHeight,
    • SubscribeEventsFromLatest
  • added the messageIndex to responses,

  • created new versions of the backend methods,

  • modified functional and integration tests by covering new endpoints.

As part of this PR:

In this PR, a bug with missing block timestamps in event responses has been fixed (#4911).

@codecov-commenter
Copy link

codecov-commenter commented Mar 28, 2024

Codecov Report

Attention: Patch coverage is 41.95402% with 101 lines in your changes are missing coverage. Please review.

Project coverage is 55.59%. Comparing base (1aab1ef) to head (d0b6949).

Files Patch % Lines
engine/access/state_stream/backend/handler.go 32.53% 53 Missing and 3 partials ⚠️
...gine/access/subscription/execution_data_tracker.go 0.00% 19 Missing ⚠️
engine/access/subscription/subscribe_handler.go 0.00% 13 Missing ⚠️
...ine/access/state_stream/backend/event_retriever.go 28.57% 8 Missing and 2 partials ⚠️
...gine/access/state_stream/backend/backend_events.go 85.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5602      +/-   ##
==========================================
- Coverage   55.66%   55.59%   -0.07%     
==========================================
  Files        1041     1042       +1     
  Lines      101935   102002      +67     
==========================================
- Hits        56741    56708      -33     
- Misses      40839    40934      +95     
- Partials     4355     4360       +5     
Flag Coverage Δ
unittests 55.59% <41.95%> (-0.07%) ⬇️

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

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

@UlyanaAndrukhiv UlyanaAndrukhiv marked this pull request as ready for review March 28, 2024 13:29
@Guitarheroua Guitarheroua self-requested a review April 2, 2024 12:31
Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

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

not finished my review, but here's some early feedback. overall I this is looking good so far

engine/access/subscription/subscribe_handler.go Outdated Show resolved Hide resolved
engine/access/state_stream/backend/handler.go Outdated Show resolved Hide resolved
engine/access/state_stream/backend/handler.go Outdated Show resolved Hide resolved
engine/access/state_stream/backend/handler.go Outdated Show resolved Hide resolved
@UlyanaAndrukhiv
Copy link
Contributor Author

UlyanaAndrukhiv commented Apr 3, 2024

@peterargue, thank you for your feedback. I have updated the pull request accordingly.

I would greatly appreciate your guidance on how to address the situation where added the 'deprecated' option to the rpc call and utilized this call in tests, as it has now caused the lint process to fail in the integration test with SA1019 staticcheck

@peterargue
Copy link
Contributor

@peterargue, thank you for your feedback. I have updated the pull request accordingly.

I would greatly appreciate your guidance on how to address the situation where added the 'deprecated' option to the rpc call and utilized this call in tests, as it has now caused the lint process to fail in the integration test with SA1019 staticcheck

does nolint work to suppress it?

[code that causes failure] //nolint: staticcheck

Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

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

added a few small comments, but otherwise this looks good.

engine/access/state_stream/backend/handler.go Outdated Show resolved Hide resolved
engine/access/subscription/subscribe_handler.go Outdated Show resolved Hide resolved
BlockID: b.ID(),
Height: b.Header.Height,
Events: expectedEvents,
BlockTimestamp: b.Header.Timestamp,
Copy link
Contributor

Choose a reason for hiding this comment

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

can you include MessageIndex here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, MessageIndex tracking has been relocated to the handler, so it isn't directly accessible here in the backend.

@peterargue peterargue requested a review from durkmurder April 4, 2024 00:04
Copy link
Contributor

@Guitarheroua Guitarheroua left a comment

Choose a reason for hiding this comment

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

Looks good for me.

@peterargue peterargue added this pull request to the merge queue Apr 4, 2024
Merged via the queue into onflow:master with commit 0b0592c Apr 4, 2024
55 checks passed
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.

4 participants