Skip to content
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

feat(logging)!: increase DefaultEntryByteThreshold to 8Mb #4247

Merged
merged 3 commits into from Jun 29, 2021

Conversation

0xSage
Copy link
Contributor

@0xSage 0xSage commented Jun 11, 2021

Fixes: #3823

DefaultEntryByteThreshold = 1 << 20 // 1MiB ===> 1 << 23 // 8MiB since size of an entries.write request can be up to 10 MB.

DefaultBufferedByteLimit is unchanged as that's more a cap on memory utilization.

@0xSage 0xSage self-assigned this Jun 11, 2021
@0xSage 0xSage requested review from a team as code owners June 11, 2021 04:56
@snippet-bot
Copy link

snippet-bot bot commented Jun 11, 2021

No region tags are edited in this PR.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 11, 2021
@0xSage
Copy link
Contributor Author

0xSage commented Jun 11, 2021

@loburm mind sanity checking this? I think changing default thresholds may produce unforeseen results, so we might want to release this with preview

@0xSage 0xSage changed the title feat: increase DefaultEntryByteThreshold to 8Mb feat(logging): increase DefaultEntryByteThreshold to 8Mb Jun 11, 2021
@0xSage 0xSage added the priority: p2 Moderately-important priority. Fix may not be included in next release. label Jun 11, 2021
@product-auto-label product-auto-label bot added the api: logging Issues related to the Cloud Logging API. label Jun 11, 2021
@loburm
Copy link
Contributor

loburm commented Jun 11, 2021

In overall it looks good to me.

@0xSage
Copy link
Contributor Author

0xSage commented Jun 14, 2021

@codyoss can I get your pointers, this PR is marked as a breaking change in go116 because:

The following breaking changes were found:
cloud.google.com/go/logging:
- DefaultEntryByteThreshold: value changed from 1048576 to 8388608

It's technically not since we're increasing the threshold, not decreasing it. How do I override this, or fix this test? Thank you!

@codyoss
Copy link
Member

codyoss commented Jun 14, 2021

It is a breaking change. I silly example is if someone wrote a test validate its value, or grabbed an element of an array at this index, which could panic if the array was not large enough. That being said I with proper usage I think this is probably fine ... @tbpg would you agree?

To get around this check the commit message needs to be formatted in a special manner:

func checkAllowBreakingChange(commit string) bool {
if strings.Contains(commit, "BREAKING CHANGE:") {
log.Println("Not running apidiff because description contained tag BREAKING_CHANGE.")
return true
}
split := strings.Split(commit, "\n")
for _, s := range split {
if strings.Contains(s, "!:") || strings.Contains(s, "!(") {
log.Println("Not running apidiff because description contained breaking change indicator '!'.")
return true
}
}
return false
}

Also, the subsequent release would need to be manual overridden as it will propose a v2.

@0xSage 0xSage changed the title feat(logging): increase DefaultEntryByteThreshold to 8Mb feat!(logging): increase DefaultEntryByteThreshold to 8Mb Jun 29, 2021
@0xSage 0xSage added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels Jun 29, 2021
@kokoro-team kokoro-team removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jun 29, 2021
@0xSage 0xSage added the automerge Merge the pull request once unit tests and other checks pass. label Jun 29, 2021
@0xSage 0xSage changed the title feat!(logging): increase DefaultEntryByteThreshold to 8Mb feat(logging)!: increase DefaultEntryByteThreshold to 8Mb Jun 29, 2021
@gcf-merge-on-green gcf-merge-on-green bot merged commit f32dd72 into master Jun 29, 2021
@gcf-merge-on-green gcf-merge-on-green bot deleted the fix3823 branch June 29, 2021 17:02
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging Issues related to the Cloud Logging API. cla: yes This human has signed the Contributor License Agreement. priority: p2 Moderately-important priority. Fix may not be included in next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

logging: default bundler limits doesn't match official API
5 participants