Skip to content

Conversation

@gusinacio
Copy link
Contributor

Signed-off-by: Gustavo Inacio [email protected]

@gusinacio gusinacio force-pushed the gustavo/tap-306-create-middleware-for-metrics branch from ae8e1f3 to cd8d333 Compare November 20, 2024 22:49
@github-actions
Copy link
Contributor

github-actions bot commented Nov 20, 2024

Pull Request Test Coverage Report for Build 11943090298

Details

  • 406 of 522 (77.78%) changed or added relevant lines in 11 files are covered.
  • 27 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+1.2%) to 73.364%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/service/src/middleware/inject_allocation.rs 70 71 98.59%
crates/service/src/middleware/inject_receipt.rs 33 34 97.06%
crates/service/src/middleware/inject_sender.rs 52 53 98.11%
crates/service/src/middleware/metrics.rs 143 145 98.62%
crates/service/src/middleware/inject_labels.rs 76 82 92.68%
crates/monitor/src/deployment_to_allocation.rs 0 10 0.0%
crates/service/src/error.rs 0 14 0.0%
crates/watcher/src/lib.rs 0 21 0.0%
crates/service/src/routes/request_handler.rs 0 30 0.0%
crates/service/src/service/indexer_service.rs 0 30 0.0%
Files with Coverage Reduction New Missed Lines %
crates/service/src/service.rs 27 0.0%
Totals Coverage Status
Change from base Build 11924905258: 1.2%
Covered Lines: 5930
Relevant Lines: 8083

💛 - Coveralls

@gusinacio gusinacio force-pushed the gustavo/tap-306-create-middleware-for-metrics branch from cd8d333 to cd38590 Compare November 20, 2024 22:51
@gusinacio gusinacio changed the title refactor: create inject_deployment middleware refactor: create metrics middleware Nov 20, 2024
@gusinacio gusinacio force-pushed the gustavo/tap-306-create-middleware-for-metrics branch from cd38590 to df9cd5c Compare November 20, 2024 22:53
@gusinacio gusinacio force-pushed the gustavo/tap-306-create-middleware-for-metrics branch 4 times, most recently from 27ccd1d to a3dce07 Compare November 21, 2024 00:21
Copy link
Member

@anirudh2 anirudh2 left a comment

Choose a reason for hiding this comment

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

Lots of interesting changes! Nice work! A lot of my requests are just for documenting public functions

@gusinacio gusinacio force-pushed the gustavo/tap-306-create-middleware-for-metrics branch from a3dce07 to d9610c8 Compare November 22, 2024 00:07
@gusinacio gusinacio requested a review from anirudh2 November 22, 2024 00:07
@gusinacio gusinacio force-pushed the gustavo/tap-306-create-middleware-for-metrics branch from d9610c8 to 1e6ad79 Compare November 22, 2024 00:12
Signed-off-by: Gustavo Inacio <[email protected]>
@gusinacio gusinacio force-pushed the gustavo/tap-306-create-middleware-for-metrics branch from 1e6ad79 to 73bfe0d Compare November 22, 2024 00:22
Copy link
Member

@anirudh2 anirudh2 left a comment

Choose a reason for hiding this comment

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

Few small things left

use tower::ServiceExt;

#[tokio::test]
async fn test_receipt_middleware() {
Copy link
Member

Choose a reason for hiding this comment

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

Reiterating this request! Just with all extensions empty.

@gusinacio gusinacio requested a review from anirudh2 November 22, 2024 00:41
Copy link
Member

@anirudh2 anirudh2 left a comment

Choose a reason for hiding this comment

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

Thanks, Gustavo!

@gusinacio gusinacio merged commit e576581 into main Nov 22, 2024
10 checks passed
@gusinacio gusinacio deleted the gustavo/tap-306-create-middleware-for-metrics branch November 22, 2024 00:43
) -> Receiver<HashMap<DeploymentId, Address>> {
map_watcher(indexer_allocations_rx, move |allocation| {
allocation
.iter()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use into_iter() here to get rid of deref in *address?

let allocation = receipt.message.allocation_id;
request.extensions_mut().insert(Allocation(allocation));
} else if let Some(deployment_id) = request.extensions().get::<DeploymentId>() {
if let Some(allocation) = my_state
Copy link
Contributor

Choose a reason for hiding this comment

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

Shoudnt it log when allocation is not found, or either the same for SignedReceiptorDeploymentId`?, for example

// adding this to the end of the ifs
         else {
            tracing::warn!("No allocation found for deployment: {:?}", deployment_id);
        }
    } else {
        tracing::warn!("Neither SignedReceipt nor DeploymentId was found in request extensions.");
    }

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