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

Enable destination_project_quota for logs exporter #579

Merged

Conversation

damemi
Copy link
Member

@damemi damemi commented Feb 2, 2023

Fixes #558

Adding this was pretty simple, the current logic builds a single LogEntry list and then batches those entries as needed to keep each batch request under the GCP size limit. We can do this without caring about the project for each entry because, while batch LogEntry requests do have a field for project, the request will honor individual project IDs set on each entry in the batch. (Traces and Metrics don't have that)

However, adding destination project quota brings us back to caring about the project at the request/batch level. That's easy to do using a map[string]LogEntry, which is similar to how traces and metrics batch.

But, it would be optimal to revert to the efficient, possibly mixed-project batching when destination_project_quota is disabled. The best I could think of to do this is to drop everything in a single key in the map, and reuse the rest of the logic. I'm not very satisfied with that, because I don't like using a magic key and it couples the destination_project_quota config option to multiple places in the code. So this might be more refactorable, but for unblocking the feature this will do fine imo, but I'm open to ideas

edit: to get CI to pass I had to add the service usage consumer role to our GCP service account in the second project

}
],
"partialSuccess": true
},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note that this fixture differs from logs_multi_project_expected.json here, with 2 lists of entries

@codecov
Copy link

codecov bot commented Feb 2, 2023

Codecov Report

Merging #579 (0c934b2) into main (e169468) will decrease coverage by 0.17%.
The diff coverage is 6.97%.

❗ Current head 0c934b2 differs from pull request most recent head 46a5f41. Consider uploading reports for the commit 46a5f41 to get more accurate results

@@            Coverage Diff             @@
##             main     #579      +/-   ##
==========================================
- Coverage   64.12%   63.95%   -0.17%     
==========================================
  Files          38       38              
  Lines        4318     4334      +16     
==========================================
+ Hits         2769     2772       +3     
- Misses       1432     1445      +13     
  Partials      117      117              
Impacted Files Coverage Δ
exporter/collector/logs.go 39.07% <0.00%> (-1.63%) ⬇️
...lector/integrationtest/testcases/testcases_logs.go 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@damemi damemi requested a review from dashpole February 2, 2023 17:50
exporter/collector/logs.go Outdated Show resolved Hide resolved
Copy link
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great.

exporter/collector/logs.go Outdated Show resolved Hide resolved
@damemi
Copy link
Member Author

damemi commented Feb 3, 2023

@damemi damemi merged commit 6445f66 into GoogleCloudPlatform:main Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor collector logs exporter to support multi-project quota
2 participants