add log10_tags context manager and with_tags decorator #299
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wenzhe-log10
commented
Sep 18, 2024
wenzhe-log10
force-pushed
the
wenzhe/eng-1148-set-default-tags
branch
from
1a48640
to
01cb17c
Compare
wenzhe-log10
changed the title
wenzhe-log10
force-pushed
the
wenzhe/eng-1148-set-default-tags
branch
from
537a482
to
0338113
Compare
kxtran
approved these changes
Sep 23, 2024
log10/load.py
Outdated
| yield | ||
|
|
||
|
|
||
| def with_tags(tags: list[str]): |
There was a problem hiding this comment.
I'd also add prefix to the with_tags func like with_log10_tags
log10/load.py
Outdated
| tags_var = contextvars.ContextVar("tags", default=[]) | ||
| extra_tags_var = contextvars.ContextVar("extra_tags", default=[]) |
There was a problem hiding this comment.
do we still need this extra_tags_var?
There was a problem hiding this comment.
removed extra_tags_var
log10/load.py
Outdated
| return None | ||
|
|
||
| if not isinstance(tags, list): | ||
| logger.error("tags must be a list") |
There was a problem hiding this comment.
would it make sense to have it as warning instead? Can you also add the message something like we're going to skip recording tags but still send the logs?
There was a problem hiding this comment.
updated the message.
log10/load.py
Outdated
| validated_tags = [] | ||
| for tag in tags: | ||
| if not isinstance(tag, str): | ||
| logger.warning(f"All tags must be strings, found {tag} of type {type(tag)}") |
There was a problem hiding this comment.
updated the message.
kxtran
reviewed
Sep 23, 2024
log10/load.py
Outdated
Comment on lines
116
to
117
| # is this tags_var for log10_session? | ||
| # first how does this tags_var get set and passed around? what's the option to set it by users |
There was a problem hiding this comment.
do we still need this comment?
nqn
approved these changes
Sep 25, 2024
- validate tags are list of strings, and skip a tag if it's not string
- rename with_tags to with_log10_tags - update logger warning message for invalid tags
wenzhe-log10
force-pushed
the
wenzhe/eng-1148-set-default-tags
branch
from
e3b014a
to
07b6b26
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.