[WIP] Add Google Cloud Logging encoding to pubsub receiver #25175
Conversation
| // TranslateLogEntry translates a JSON-encoded LogEntry message into a pair of | ||
| // pcommon.Resource and plog.LogRecord, trying to keep as close as possible to | ||
| // the semantic conventions. | ||
| // | ||
| // For maximum fidelity, the decoding is done according to the protobuf message | ||
| // schema; this ensures that a numeric value in the input is correctly | ||
| // translated to either an integer or a double in the output. It falls back to | ||
| // plain JSON decoding if payload type is not available in the proto registry. |
There was a problem hiding this comment.
Following the proto schema is responsible for a fair amount of the code here. It's arguably not worth the effort to avoid a few mistranslated numeric values. 🤷
There was a problem hiding this comment.
I just remembered that this isn't just about int vs float, it's also important for bytes, durations, and timestamps, all of which are both encoded as strings according to the protobuf json mapping. Actually checking that, it seems that all integers are to be encoded as strings, but can be decoded from either string or number.
There was a problem hiding this comment.
additionally there should be an optional metric for unresolvable type uris so that people can know to add them if needed.
| import ( | ||
| _ "google.golang.org/genproto/googleapis/cloud/audit" | ||
| ) |
There was a problem hiding this comment.
I'd prefer to replace or augment this with functionality I'm hoping to propose in the builder, which allows adding go packages to be imported for side effects. this would let users add types they want accurate conversion for.
that is implemented here: kamalmarhubi/opentelemetry-collector@d846d4d
I initially wanted that for an approach I was considering for custom ottl functions: #16098 (comment)
but it would help here too, I think?
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
not stale |
|
@kamalmarhubi I'm starting to work on this; I've checked out the branch locally (almost ready to test). Could you rebase against main, so it will be in a better state to merge. |
| switch k { | ||
| // Unpack as suggested in the logs data model appendix | ||
| // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model-appendix.md#google-cloud-logging | ||
| case "timestamp": |
There was a problem hiding this comment.
I would add LovEntry.receiveTimestamp, and map it to Proto(observed_time_unix_nano).
| return res, lr, err | ||
| } | ||
| delete(src, k) | ||
| default: |
There was a problem hiding this comment.
LogEntry.insertId can be mapped to Attribute(log.record.uid) (see https://opentelemetry.io/docs/specs/otel/logs/semantic_conventions/general/)
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Not stale... if the original author wants that I take over the branch and refactor I'm ok with that. @kamalmarhubi |
kamalmarhubi
force-pushed
the
pubsub-google-cloud-logging
branch
from
73c7a80 to
51ae1da
Compare
Add a new `cloud_logging` encoding, which allows the receiver to consume messages from a Pub/Sub topic that's used as the destination of a Cloud Logging sink. refs open-telemetry#23184
|
@alexvanboxel sorry for the slow response! I just rebased and pushed the branch, including your two recommendations (insertId, receiveTimestamp), and a very brief addition to the README. I haven't had time to add more realistic tests yet. If you're willing to take over the PR that works for me. Otherwise I'll do what I can! |
kamalmarhubi
force-pushed
the
pubsub-google-cloud-logging
branch
from
51ae1da to
4e84241
Compare
|
@alexvanboxel a couple of thoughts / questions:
|
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Not stale, I would keep it MonitoredResource translation as is. If you don't mind, I'll keep the branch up-to-date and do some tweaks. I'll make sure to keep you credited as contributor. |
absolutely fine with me! let me know if there's anything else you'd like on my end. otherwise looking forward to this making it in :-) |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
as agreed, I'll take over the branch (I will keep credits for the original commit to @kamalmarhubi). The branch can be found here: #29299 The branch is rebased against the latest release. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
@kamalmarhubi can you close this draft, as the branch #29299 is being pushed through |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
|
Ah woops meant to close this since it was superseded by #29299 |
Not for review yet!
refs #23184
Description:
Link to tracking Issue:
Testing:
Documentation: