-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[Metricbeat] GCP cloudsql metadata #33066
Conversation
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
@tommyers-elastic May you have a look at this comment:
and provide your opinion here? We can either try to fix this here, leave it like this for consistency or try to refactor (we would need an exploration phase to understand how this would play out) |
Sorry I missed the comment here. I'm fine with leaving the fields under |
@gpop63 I was reviewing this contribution, there is only one thing that we need to double check: this code uses the same caching logic as Can you please remove the caching logic from this module? It will be reintroduced if the need arise in future updates. |
I have a question regarding
|
@gpop63 If that is the GCP provided ID I would move forward with it. We need to make sure the mapping in the data stream is correct though, or there could be errors during ingestion. |
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.
Adding a small comment to clarify the role of instances
.
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.
looks good to me, could you add a changelog entry please? Thank you!
// cloudsqlMetadata is an object to store data in between the extraction and the writing in the destination (to uncouple | ||
// reading and writing in the same method) | ||
type cloudsqlMetadata struct { | ||
// projectID string |
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 remove the projectID, machineType and ts here?
Actually looking at this comment, I don't think we should use |
* add cloudsql metadata * add cloudsql metadata tests * add cloudsql service constant * add license headers * mage update * fix golangci-lint * remove cache * add instance name field * add instances comment * add import aliases * add changelog entry * remove cloud instance and unused metadata fields Co-authored-by: kaiyan-sheng <kaiyan.sheng@elastic.co>
What does this PR do?
While working on the GCP CloudSQL integration elastic/integrations#4126 I discovered that there is no way to know which database type (PostgreSQL, MySQL or SQLServer) generated an event.
Adds
cloudsql
metadata.An event would look like this with the added
cloudsql
labels.@kaiyan-sheng suggested adding the
cloudsql
metadata undergcp
not underlabels
but the stackdriver metadata only allows access toECS
andLabels
.Why is it important?
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Author's Checklist
How to test this PR locally
Related issues
Use cases
Screenshots
Logs