Page MenuHomePhabricator

[services] Backup - Tests - Pull Backup - Improve logging
ClosedPublic

Authored by karol on Sep 1 2022, 4:05 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 22, 11:58 AM
Unknown Object (File)
Fri, Nov 22, 5:54 AM
Unknown Object (File)
Tue, Nov 19, 9:33 PM
Unknown Object (File)
Mon, Nov 18, 10:44 AM
Unknown Object (File)
Sat, Nov 16, 4:31 PM
Unknown Object (File)
Fri, Nov 15, 7:04 AM
Unknown Object (File)
Sun, Nov 10, 6:58 AM
Unknown Object (File)
Sat, Nov 2, 5:46 AM

Details

Summary

Making logs clearer in the pull backup test utility

Test Plan
cd services
yarn run-integration-tests backup

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

karol edited the test plan for this revision. (Show Details)
karol added reviewers: tomek, varun, max.
jon requested changes to this revision.Sep 1 2022, 8:32 AM
jon added inline comments.
services/commtest/tests/backup/pull_backup.rs
123

These should really be debug! or info!.

This revision now requires changes to proceed.Sep 1 2022, 8:32 AM
karol added inline comments.
services/commtest/tests/backup/pull_backup.rs
123

I couldn't make it work, I'll wait for @varun to get back from the holidays to sync and talk about this.

There's a task for this: https://linear.app/comm/issue/ENG-1717/use-tracing-utilities-instead-of-println

jon requested changes to this revision.Sep 2 2022, 10:26 AM
jon added inline comments.
services/commtest/tests/backup/pull_backup.rs
123

the log crate just exposes an api, there's several logger crates which then allow that API to be used, like https://docs.rs/env_logger/0.9.0/env_logger/

This revision now requires changes to proceed.Sep 2 2022, 10:26 AM

Why do you keep blocking this? I created and linked a linear task for this. I think this is not a high priority, this can be done anytime by anyone on the team, it's better to first focus on crucial functionalities and only when they work, finish up by polishing the details, especially that we're going through unknown a little bit.

Please, do not mind this problem anymore here, and let's follow up on the task, if you feel, however, like this should be done pronto with the biggest priority, why don't you specify why is that exactly? Because it doesn't add up for me, sorry.

@jon what is the ETA of the review here? This is the very first diff in the stack so it should be prioritized, right? Thanks!

Why do you keep blocking this?

Because it's an anti-pattern, and I don't think it belongs in the code.

I created and linked a linear task for this.

The "create a ticket" process makes sense when the ask increases the amount of work. I'm asking to do the canonical logging thing.

I think this is not a high priority, this can be done anytime by anyone on the team

Agreed, but why create work for others?

it's better to first focus on crucial functionalities

I don't agree with this. Maintaining a high degree of polish allows you to do temporary sprints to push through features. If you're just constantly pushing through features and accumulating technical debt that's how we get into situations where you try to push a feature and it takes way longer to complete anything.

and only when they work, finish up by polishing the details

It's even better to have the details and standard processes established beforehand, so everyone is aligned and not creating additional technical debt for others.

especially that we're going through unknown a little bit.

Sure, but we're talking about logging practices, not proof-of-concept features.

All this being said, I'll approve this, but work on making it in-excusable to not use canonical rust logging.

This revision is now accepted and ready to land.Sep 7 2022, 10:13 AM

Agreed, but why create work for others?

You didn't understand. I said:

I think this is not a high priority, this can be done anytime by anyone on the team

It can be done, not it has to be done by anyone on the team. It's about a possibility (it's so easy that it's not a problem to defer this to someone else), not about "creating work for others".

The "create a ticket" process makes sense when the ask increases the amount of work. I'm asking to do the canonical logging thing.

It does increase the amount of work. You didn't pay attention to what I said. I said earlier:

I couldn't make it work, I'll wait for @varun to get back from the holidays to sync and talk about this.

That means that I tried to do it quickly, but it didn't work, and I decided it's not worth spending time on this right now.

I don't agree with this. Maintaining a high degree of polish allows you to do temporary sprints to push through features. If you're just constantly pushing through features and accumulating technical debt that's how we get into situations where you try to push a feature and it takes way longer to complete anything.

Usually, this is right, but I don't see a big technical debt in replacing all occurrences of println! with something else (right now there's exactly 1 occurrence). It's all about prioritization, which you may think isn't the best, but I'm going to have to ask you to trust me on this. My goal was to make the services operational ASAP using the rust layer. After the POC works, we can nitpick and refactor the code as much as we want.