Add Google Pubsub receiver for OTLP messages - PR1 #3552
Conversation
alexvanboxel
force-pushed
the
feature/googlecloudpubsubreceiver-pr1
branch
4 times, most recently
from
ba0075f
to
80bf1cf
Compare
|
Rebased, squached and added me to code owners, and added ependabot. /cc @punya @dashpole @bogdandrutu |
alexvanboxel
force-pushed
the
feature/googlecloudpubsubreceiver-pr1
branch
from
80bf1cf
to
6169930
Compare
|
Comments about community module added (but for receiver), note that following the guidelines the implementation and the switch are upcoming |
| case "": | ||
| case "otlp_proto_trace": | ||
| default: | ||
| return fmt.Errorf("log format should be either otlp_proto, raw_string or raw_json") |
There was a problem hiding this comment.
I'm not sure I understand this error message. Is the assumption that if a non-OTLP format is specified for traces that it must have been intended for logs? raw_string and raw_json wouldn't be valid here, would they?
There was a problem hiding this comment.
error message corrected
|
How is this PR different from #3551? The code looks very similar, and I don't want to waste your time by duplicating comments. |
This is the receiver, as this is the mandatory PR1 (as mandated for new modules) they look similar. Implementation is very different of course (but that comes later). |
alexvanboxel
force-pushed
the
feature/googlecloudpubsubreceiver-pr1
branch
from
b0e357a
to
7e4963e
Compare
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
alexvanboxel
force-pushed
the
feature/googlecloudpubsubreceiver-pr1
branch
from
7e4963e
to
189ce50
Compare
|
Rebased to the latest main and squashed everything as a lot of changes were required. I think all concerns in the comments are addressed. |
alexvanboxel
force-pushed
the
feature/googlecloudpubsubreceiver-pr1
branch
2 times, most recently
from
ed9cc7d
to
f26cb19
Compare
alexvanboxel
force-pushed
the
feature/googlecloudpubsubreceiver-pr1
branch
from
f26cb19
to
53b1419
Compare
|
@bogdandrutu can you also approve the receiver part? |
|
Can someone approve this, @bogdandrutu approved the exporter ( #3551 ). This PR is blocking me to go to phase 2 of merging the actual implementation. |
|
Should the title of this PR be "PR2" since #3551 is "PR1"? |
|
@Aneurysm9 you already reviewed this, PTAL. |
|
This is the receiver: receiver!, not the exporter, (the exporter is already merged), all are PR1 base boilerplate commits. |
I will, thanks |
Co-authored-by: Anthony Mirabella <[email protected]>
|
@Aneurysm9 can you press the merge button? I'm not authorized todo so. |
| @@ -0,0 +1,63 @@ | |||
| # Google Pubsub Receiver | |||
There was a problem hiding this comment.
Hi Alexvanboxel, nice to meet you! I'm a member of the Cloud Trace team at Google.
Cloud Trace supports the single receiver approach, with one caveat. Our public documentation will still instruct customers to create one PubSub topic per telemetry type. The multi-topic approach allows for a more fine-grained authorization model (whereby Trace can only publish to the trace topic, Logging can only publish to the logging topic, and so on). However, this shouldn't affect your implementation. You can still write a single receiver that has the ability to accept all telemetry types. Our public documentation can simply encourage "best" security practices. There would be no inherent technical limitation that would prevent the customer from ignoring our documentation and piping all telemetry types through a single PubSub topic anyways.
We also favor this CloudEvents approach (as opposed to the OneOf() approach discussed in PR #157), implemented as Pub/Sub attributes. Our reasoning is that customers should be able to filter by telemetry type using message attributes before needing to deserialize the message.
One question I had, though, is your reasoning for using the CloudEvents spec? I'm unfamiliar with the spec, but reading through it, it appears rather thorough. Is the expectation that the receiver (and Trace, the publisher) will be fully compliant with the spec? And if not, and we are only using portions of it for inspiration, then does it make sense to give the PubSub attributes more familiar names such as (just as an example):
- "payload_format":"binaryproto"
- "payload_message":"opentelemetry.proto.trace.v1.ResourceSpans"
Finally, I wanted to thank you for your contributions! Let me know if you have any other questions for the Trace team.
Thanks,
Matt
There was a problem hiding this comment.
Hey, Matt: it's maybe worth having a meeting, last time I had a meeting with "Eyamba Ita" there was no mention on OTLP over Pubsub. You can mail me at "alex.vanboxel", at the company I work for "collibra.com". <- email spam bot proof
* Add Google Pubsub receiver for OTLP messages * Update receiver/googlecloudpubsubreceiver/config.go Co-authored-by: Anthony Mirabella <[email protected]> Co-authored-by: Anthony Mirabella <[email protected]>
Description:
Add the skeleton for Google Pubsub receiver for OTLP messages. The export is able to read OTEP messages to a Google Pubsub subscription.
This receiver is the 2nd rewrite following feedback from the community on the original PR ( #1892 ), and also implements feedback given in the OTLP/Messaging otep:
open-telemetry/oteps#157
PR1 as described in the CONTRIBUTING guide only the README, config, and factory.
Link to tracking Issue:
#1802
Testing:
Tests for config and factory
Documentation:
Up-to-date README