Add BaggageLogProcessor to opentelemetry-processor-baggage#4371
Add BaggageLogProcessor to opentelemetry-processor-baggage#4371Manvi2402 wants to merge 1 commit intoopen-telemetry:mainfrom
Conversation
|
|
||
| def on_emit(self, log_record: ReadWriteLogRecord) -> None: | ||
| baggage = get_all_baggage() | ||
| for key, value in baggage.items(): |
There was a problem hiding this comment.
It would be good to add collision handling here. It could be that a key in baggage is the same as a key added from stdlib logging, calls to logging.emit, or a user's custom LogRecordProcessor
There was a problem hiding this comment.
Thanks @Manvi2402 for adding that. Could this also be documented, either in the class or the function docstring?
| baggage = get_all_baggage() | ||
| for key, value in baggage.items(): | ||
| if self._baggage_key_predicate(key): | ||
| log_record.log_record.attributes[key] = value |
There was a problem hiding this comment.
Could there also be a configurable limit on how many attributes are added? Logs can take up a lot of storage quickly
| values will appear in all outgoing HTTP headers from the application. | ||
| """ | ||
|
|
||
| def __init__(self, baggage_key_predicate: BaggageKeyPredicateT) -> None: |
There was a problem hiding this comment.
Could this support multiple predicates? For example if user wants an allow-list for more heterogeneous keys that don't all have the same prefix/suffix.
|
Thanks @Manvi2402 for submitting. I think this is a nice analog feature and iiuc shouldn't interfere with the Logging API stabilization efforts. I've left some comments. Please could you also:
|
Manvi2402
force-pushed
the
add-baggage-log-processor
branch
from
176f8d3 to
3449a3d
Compare
|
Thanks for the review @tammy-baylis-swi . I've addressed all the comments in the latest commit 3449a3d Added collision handling to avoid overwriting existing log record attributes. |
|
|
||
| ### Added | ||
| - Add `BaggageLogProcessor` to `opentelemetry-processor-baggage` | ||
| ([#4062](https://github.com/open-telemetry/opentelemetry-python-contrib/issues/4062)) |
There was a problem hiding this comment.
Nit: our convention is to link the PRs
| ([#4062](https://github.com/open-telemetry/opentelemetry-python-contrib/issues/4062)) | |
| ([#4371](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/4371)) |
| def __init__( | ||
| self, | ||
| baggage_key_predicate: BaggageKeyPredicatesT, | ||
| max_baggage_attributes: Optional[int] = None, |
There was a problem hiding this comment.
The spec outlines 128 as a max for attributes though not specific to baggage. Maybe we could use this as a default?
| ) | ||
|
|
||
|
|
||
| class BaggageLogProcessorTest(unittest.TestCase): |
There was a problem hiding this comment.
Please add another test case for supporting multiple predicates, and also update README with an example
2 participants
Description
Adds
BaggageLogProcessorwhich reads entries stored in Baggage from the current context and adds the baggage entries' keys and values to the log record as attributes on emit.This is analogous to the existing
BaggageSpanProcessorbut for logs.Fixes #4062
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Added unit tests in
test_baggage_log_processor.py:test_check_the_baggage- verifies BaggageLogProcessor is instance of LogRecordProcessortest_baggage_added_to_log_record- verifies baggage is added to log attributestest_baggage_with_prefix- verifies predicate filtering with string prefixtest_baggage_with_regex- verifies predicate filtering with regextest_no_baggage_not_added- verifies no baggage keys added when baggage is emptyDoes This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.