search

GoogleCloudPlatform / cloud-logging-data-source-plugin

https://grafana.com/grafana/plugins/googlecloud-logging-datasource/
Apache License 2.0
17 stars 12 forks source link

Adds support for AuditLogs #39

Closed StefanKurek closed 5 months ago

StefanKurek commented 1 year ago

Attempts to convert the protobuf body of Audit logs to JSON for easier viewing. Also attempts to convert all body key/values of Audit logs to labels on the log. Filtering on these new labels should properly add a new filter to the query.

xiangshen-dk commented 1 year ago

Sorry, I didn't see your PR and added the similar feature in mine: https://github.com/GoogleCloudPlatform/cloud-logging-data-source-plugin/pull/40

Since my PR has some extra things (I probably should break it to smaller ones), maybe you can review mine? Thanks!

StefanKurek commented 1 year ago

Sorry, I didn't see your PR and added the similar feature in mine: #40

Since my PR has some extra things (I probably should break it to smaller ones), maybe you can review mine? Thanks!

@xiangshen-dk Hello. Just getting back from vacation. I took a quick look at the PR (which has since been merged). The parts related to Audit Logs looked ok to me. I think I only noticed two differences compared to this PR.

  1. I attempted to display the body as a JSON string rather than protobuf string. Not really necessary (and possibly not the correct approach), but generally made it easier to look at in my opinion.

  2. I created a brand new function for converting the Audit Log body to labels. Because of this I was able to handle both nested maps and arrays (I don't believe the function that you were reusing was handling arrays well. I think it just ended up dumping everything from that point forward into a single string). This allowed for proper log filtering with both map and array based properties (I had to also add a little bit of custom code to remove the [x] from the label key when adding to the query filter for it to work correctly).

I specifically think the features I had in (2) were useful (could be added to the existing label creation function to make both JSON and Protobuf labels more functional overall). Let me know what you think and if you'd like me to just close this PR. Thanks!

xiangshen-dk commented 1 year ago

Thanks for the explanation! Would you mind to update your PR and resubmit it?

StefanKurek commented 1 year ago

Thanks for the explanation! Would you mind to update your PR and resubmit it?

@xiangshen-dk would it be better to try and instead incorporate a case statement similar to my

    case []interface{}:
        for index, value := range t {
            interfaceToLabels(labels, fmt.Sprintf("%s[%s]", fieldName, strconv.Itoa(index)), value)
        }
    }

for array types to the fieldToLabels method that you guys are using? This would keep the code more condensed than my PR, and as a benefit would make the JsonPayload labels have more functionality as well.