-
Notifications
You must be signed in to change notification settings - Fork 923
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.
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
[PubSub to Datadog] Updating Datadog template to include full pubSub message by default #1579
base: main
Are you sure you want to change the base?
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
0d76602
to
bd1dfe2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you! Let's just make sure to communicate with existing users, as I believe restarting an existing Dataflow job will change behavior moving forward.
bd1dfe2
to
7d68385
Compare
7d68385
to
a4804c8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1579 +/- ##
============================================
- Coverage 44.79% 41.26% -3.53%
- Complexity 700 2884 +2184
============================================
Files 297 746 +449
Lines 15894 43633 +27739
Branches 1569 4645 +3076
============================================
+ Hits 7119 18005 +10886
- Misses 8245 24111 +15866
- Partials 530 1517 +987
|
@@ -40,7 +40,7 @@ on [Metadata Annotations](https://github.com/GoogleCloudPlatform/DataflowTemplat | |||
* **apiKey** : The Datadog API key. You must provide this value if the `apiKeySource` is set to `PLAINTEXT` or `KMS`. For more information, see API and Application Keys (https://docs.datadoghq.com/account_management/api-app-keys/) in the Datadog documentation. | |||
* **batchCount** : The batch size for sending multiple events to Datadog. The default is `1` (no batching). | |||
* **parallelism** : The maximum number of parallel requests. The default is `1` (no parallelism). | |||
* **includePubsubMessage** : Whether to include the full Pub/Sub message in the payload. The default is `false` (only the data element is included in the payload). | |||
* **includePubsubMessage** : Whether to include the full Pub/Sub message in the payload. The default is `true` (all elements, including data element, are included in the payload). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually do not recommend changing the default value. What if we just update the doc to mention setting this to True is recommended?
@@ -65,7 +65,7 @@ variable "parallelism" { | |||
|
|||
variable "includePubsubMessage" { | |||
type = bool | |||
description = "Include full Pub/Sub message in the payload (true/false). Defaults to false (only data element is included in the payload)." | |||
description = "Include full Pub/Sub message in the payload (true/false). Defaults to true (all elements, including data element, are included in the payload)." | |||
default = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we update the default here as well?
Purpose:
Given customer feedback and to help improve parity with legacy logging structure updating default
includePubsubMessage
option to be true.