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

vdk-structlog: add default logging format values #3055

Merged
merged 1 commit into from
Feb 12, 2024
Merged

vdk-structlog: add default logging format values #3055

merged 1 commit into from
Feb 12, 2024

Conversation

DeltaMichael
Copy link
Contributor

@DeltaMichael DeltaMichael commented Jan 26, 2024 •
edited
Loading

Why?

For users who do not wish to configure each structlog option separately, introduce presets for different cases.

What?

Introduce CLOUD and LOCAL presets.

The CLOUD preset hardcodes the following options

export VDK_STRUCTLOG_FORMAT=console
export VDK_STRUCTLOG_CONSOLE_LOG_PATTERN="%(asctime)s [VDK] %(vdk_job_name)s [%(levelname)-5.5s] %(name)-30.30s %(filename)20.20s:%(lineno)-4.4s %(funcName)-16.16s[id:%(attempt_id)s]- %(message)s"

LOCAL just uses the defaults set in the configuration code

Refactor structlog plugin to eliminate repeating code
Remove syslog config tests
Fix bug with vdk fields crashing logging for custom format
Add tests for vdk fields with custom format

How was this tested?

Functional tests
CI

What kind of change is this?

Feature/non-breaking

@DeltaMichael DeltaMichael force-pushed the person/mdilyan/structlog-defaults branch from bc84b1f to 950d9d9 Compare January 26, 2024 08:26
@DeltaMichael DeltaMichael linked an issue Jan 26, 2024 that may be closed by this pull request
vdk-structlog: sensible defaults for metadata and output format #2811
Closed
@DeltaMichael DeltaMichael force-pushed the person/mdilyan/structlog-defaults branch from 6690ddb to dc6f444 Compare January 26, 2024 08:43
@DeltaMichael DeltaMichael force-pushed the person/mdilyan/structlog-defaults branch from dc6f444 to 4643800 Compare January 26, 2024 10:28
@sabadzhiev sabadzhiev requested a review from yonitoo January 29, 2024 09:14
yonitoo
yonitoo approved these changes Jan 29, 2024
View reviewed changes
Copy link
Contributor

@yonitoo yonitoo left a comment

Choose a reason for hiding this comment

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

Overall, LGTM. Just the bit with the log level env vars has to be checked if it could be improved. Also, at first I had a little concern about passing the whole CoreContext to some of the functions but it makes sense.

@antoniivanov
Copy link
Collaborator

I just wanted to clarify the intent behind this PR. I don't think the description makes it clear (the why section ). So I will ask to update it, please.

The way I see it the primary goal of this PR is to offer a mechanism that allows users to easily configure their logging setup, tailored specifically for cloud-based or local runs. So we introduce opinionated configuration options that we believe represent the best practices for cloud and local scenarios. So instead of users figuring how to configure all 5-10 options . They just set config_type="CLOUD" or LOCAL.

Of course these configurations are not rigid. Users retain the flexibility to modify any aspect of the logging configuration, such as the log_format, logging_metadata, to better suit their specific needs. This approach ensures a balance between providing a solid starting point and maintaining user flexibility in customization

@DeltaMichael DeltaMichael force-pushed the person/mdilyan/structlog-defaults branch 7 times, most recently from f30a1ae to f2210bd Compare January 30, 2024 14:05
@DeltaMichael DeltaMichael force-pushed the person/mdilyan/structlog-defaults branch 3 times, most recently from 51370f7 to bfa7c5c Compare February 1, 2024 14:28
DeltaMichael pushed a commit that referenced this pull request Feb 5, 2024
vdk-core: add is_default() function to config
6cd3382 
Why?

There are cases where we want to know we're using
the default value for a config option. One such
case is configuration presets

#3055

What?

Add is_default() to the config object.

How was this tested?

Unit tests

What kind of change is this?

Feature/non-breaking

Signed-off-by: Dilyan Marinov <[email protected]>
@DeltaMichael DeltaMichael mentioned this pull request Feb 5, 2024
Merged
DeltaMichael pushed a commit that referenced this pull request Feb 5, 2024
@DeltaMichael
vdk-core: add is_default() function to config
0e32258 
Why?

There are cases where we want to know we're using
the default value for a config option. One such
case is configuration presets

#3055

What?

Add is_default() to the config object.

How was this tested?

Unit tests

What kind of change is this?

Feature/non-breaking

Signed-off-by: Dilyan Marinov <[email protected]>
DeltaMichael pushed a commit that referenced this pull request Feb 6, 2024
vdk-core: add is_default() function to config
f731ad1 
Why?

There are cases where we want to know we're using
the default value for a config option. One such
case is configuration presets

#3055

What?

Add is_default() to the config object.

How was this tested?

Unit tests

What kind of change is this?

Feature/non-breaking

Signed-off-by: Dilyan Marinov <[email protected]>
DeltaMichael pushed a commit that referenced this pull request Feb 6, 2024
vdk-core: add is_default() function to config
8c69032 
Why?

There are cases where we want to know we're using
the default value for a config option. One such
case is configuration presets

#3055

What?

Add is_default() to the config object.

How was this tested?

Unit tests

What kind of change is this?

Feature/non-breaking

Signed-off-by: Dilyan Marinov <[email protected]>
DeltaMichael pushed a commit that referenced this pull request Feb 6, 2024
vdk-core: add is_default() function to config
223b807 
Why?

There are cases where we want to know we're using
the default value for a config option. One such
case is configuration presets

#3055

What?

Add is_default() to the config object.

How was this tested?

Unit tests

What kind of change is this?

Feature/non-breaking

Signed-off-by: Dilyan Marinov <[email protected]>
DeltaMichael pushed a commit that referenced this pull request Feb 6, 2024
vdk-core: add is_default() function to config
0e7073e 
Why?

There are cases where we want to know we're using
the default value for a config option. One such
case is configuration presets

#3055

What?

Add is_default() to the config object.

How was this tested?

Unit tests

What kind of change is this?

Feature/non-breaking

Signed-off-by: Dilyan Marinov <[email protected]>
DeltaMichael added a commit that referenced this pull request Feb 6, 2024
@DeltaMichael
vdk-core: add is_default() function to config (#3076)
b3e7b45 
## Why?

There are cases where we want to know we're using the default value for
a config option. One such case is configuration presets

#3055

## What?

Add is_default() to the config object.

## How was this tested?

Unit tests

## What kind of change is this?

Feature/non-breaking

Signed-off-by: Dilyan Marinov <[email protected]>
Co-authored-by: Dilyan Marinov <[email protected]>
@DeltaMichael DeltaMichael force-pushed the person/mdilyan/structlog-defaults branch 2 times, most recently from a99e745 to c53aaf0 Compare February 9, 2024 12:18
antoniivanov
antoniivanov approved these changes Feb 9, 2024
View reviewed changes
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.

It looks good to me.

The only thing I'd simplify the structlog_plugin module as I suggested below.
But I don't feel that strongly to block merging the change over that.

@DeltaMichael
vdk-structlog: add default logging format values
20cf1dc 
Why?

For users who do not wish to configure each structlog option separately, introduce presets for different cases.

What?

Introduce CLOUD and LOCAL presets.

The CLOUD preset hardcodes the following options

```sh
export VDK_STRUCTLOG_FORMAT=console
export VDK_STRUCTLOG_CONSOLE_LOG_PATTERN="%(asctime)s [VDK] %(vdk_job_name)s [%(levelname)-5.5s] %(name)-30.30s %(filename)20.20s:%(lineno)-4.4s %(funcName)-16.16s[id:%(attempt_id)s]- %(message)s"
```

LOCAL just uses the defaults set in the configuration code

Refactor structlog plugin to eliminate repeating code Remove syslog config tests
Fix bug with vdk fields crashing logging for custom format
Add tests for vdk fields with custom format

How was this tested?

Functional tests
CI

What kind of change is this?

Feature/non-breaking

Signed-off-by: Dilyan Marinov <[email protected]>
@DeltaMichael DeltaMichael force-pushed the person/mdilyan/structlog-defaults branch from c53aaf0 to 20cf1dc Compare February 12, 2024 07:56
@DeltaMichael DeltaMichael enabled auto-merge (squash) February 12, 2024 08:01
@DeltaMichael DeltaMichael merged commit d0ef338 into main Feb 12, 2024
9 checks passed
@DeltaMichael DeltaMichael deleted the person/mdilyan/structlog-defaults branch February 12, 2024 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antoniivanov antoniivanov antoniivanov approved these changes

@yonitoo yonitoo yonitoo approved these changes

Assignees
No one assigned
Labels
cla-not-required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

vdk-structlog: sensible defaults for metadata and output format
4 participants
@DeltaMichael @antoniivanov @yonitoo @vmwclabot