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

vdk-structlog: create structured logging plugin #2801

Merged
merged 1 commit into from
Oct 23, 2023

Conversation

DeltaMichael
Copy link
Contributor

@DeltaMichael DeltaMichael commented Oct 17, 2023

Why?

As part of VEP-2448, we'd like to make structured logging pluggable

What?

Create a vdk-structlog plugin. The plugin allows users to configure logging metadata and logging format. It also works with bound loggers.

How was this tested?

Installed the plugin and ran a data job locally with different configurations

White kind of change is this?

feature/non-breaking

Follow-up

@DeltaMichael DeltaMichael linked an issue Oct 19, 2023 that may be closed by this pull request
@DeltaMichael DeltaMichael force-pushed the person/mdilyan/vdk-structlog branch 4 times, most recently from e78a4d0 to b92e384 Compare October 19, 2023 14:11
Copy link
Collaborator

@antoniivanov antoniivanov left a comment

Choose a reason for hiding this comment

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

There's glaring omission of unit tests and generally any tests.

I strongly recommend avoiding manually testing changes as you develop and instead writing the tests as you write the code.

With time it is very likely to speed up your productivity significantly. You do not need go full Test-driven development (TDD) but at least partial TDD is something I really recommend - you can write some class (its first version) and then write tests for it , fix its bugs, write another class or methods or small functionality, write tests for it, once you are sure it works continue.

Copy link
Collaborator

@antoniivanov antoniivanov left a comment

Choose a reason for hiding this comment

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

As this is a POC/prototype (Development status: Pre-alpha) I am ok if we merge it and address comments in follow up PRs

@DeltaMichael
Copy link
Contributor Author

As this is a POC/prototype (Development status: Pre-alpha) I am ok if we merge it and address comments in follow up PRs

I can address most of the comments here, except for the tests. We have separate tickets for those.

@antoniivanov
Copy link
Collaborator

I can address most of the comments here, except for the tests. We have separate tickets for those.

I am ok with that. I am generally ok with POC/prototype code to have less tests. But not a single test was really strange for me. I just really feel that it's a bad habit. And I do think writing your tests as you code is more efficient than writing code and manually testing.

One comment for the future. I think you are aware. But it would not hurt for me to say it.
We generally include tests within the story itself . PR assumes it will be accompanied by sufficient tests itself.

There are cases where we have issues/stories about tests - those are when we also need to create the framework around the tests -e.i. when more design work is required upfront. Or in case of POCs where the POC had little tests.

@DeltaMichael
Copy link
Contributor Author

@antoniivanov I'm generally opposed to writing tests before a concept has solidified. Once it's solid, you can point to it and say "this is correct behaviour, we should record it as tests". This started as a POC and I had no idea where it was going to go, but it went better than expected, so we should move forward with it. I also figured adding e2e tests was too much for one PR, so I opened an e2e tests ticket, which is a blocker for further changes.

TDD is good in theory, but in practice there's very little value in unit-testing classes with very few methods that have a couple of for-loops. This is especially true if your concept is not yet solid and the unit tests are likely to change with the implementation. There's value in testing the e2e behaviour and digging down when necessary, e.g. if you're passing an algorithm as a strategy, have unit/integration tests for that, but not for every class.

@DeltaMichael DeltaMichael force-pushed the person/mdilyan/vdk-structlog branch 5 times, most recently from d58f224 to 5c3b6b3 Compare October 20, 2023 15:33
Why?

As part of VEP-2448, we'd like to make structured logging pluggable

What?

Create a vdk-structlog plugin. The plugin allows users to configure
logging metadata and logging format. It also works with bound loggers.

How was this tested?

Installed the plugin and ran a data job locally with
different configurations

White kind of change is this?

feature/non-breaking

Signed-off-by: Dilyan Marinov <mdilyan@vmware.com>
@DeltaMichael DeltaMichael enabled auto-merge (squash) October 23, 2023 07:53
@DeltaMichael DeltaMichael merged commit e2e1a01 into main Oct 23, 2023
7 checks passed
@DeltaMichael DeltaMichael deleted the person/mdilyan/vdk-structlog branch October 23, 2023 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants