Adding new cloudprober surfacer for bigquery insertion. #274
Conversation
|
@manugarg This is a PR adding a BQ surfacer that we were mentioning earlier. Please have a look and let us know for any feedback. |
Pulkit0110
force-pushed
the
bqsurfacer
branch
from
ba55795
to
1fae21c
Compare
|
@Pulkit0110 @kushal288 This PR has a lot of unrelated files. Can you please clean it up? |
Pulkit0110
force-pushed
the
bqsurfacer
branch
from
a3f8418
to
ea32f67
Compare
Pulkit0110
force-pushed
the
bqsurfacer
branch
from
ea32f67
to
753bbd8
Compare
Its done. |
|
@Pulkit0110 Thanks for fixing the PR. I just wanted to say that I've not forgotten about this PR. I'll review it soon. |
There was a problem hiding this comment.
Hey @Pulkit0110,
Thanks again for adding this surfacer. Please take a look at the inline comments.
There was a problem hiding this comment.
I think something is missing in this surfacer. I don't see you making use of the metrics values. Just to make sure we're on the same page, here is the EventMetrics code:
https://github.com/cloudprober/cloudprober/blob/master/metrics/eventmetrics.go
Also, this code may benefit from looking at the postgres surfacer:
https://github.com/cloudprober/cloudprober/tree/master/surfacers/postgres
|
@Pulkit0110 I'll take a look at it over the weekend. |
|
@manugarg did you get a chance to look at the latest changes? |
There was a problem hiding this comment.
@Pulkit0110, Sorry for the delay. This code is a bit hard to follow, and quite unusual in many ways. I've tried my best to figure out what's going and added my comments. Let me know if they don't make sense to you.
|
@manugarg I've resolved all the comments. Can you have a look and let me know if more changes are required? |
|
@manugarg , did you get a chance to take a look at the latest changes? |
|
Thanks for recent changes, @Pulkit0110! I've started reviewing it. Give me a couple of more days to finish. |
There was a problem hiding this comment.
Hi Pulkit,
It looks much better. Thank you for all the changes.
I added a few more comments, but most of them are suggestions except for the metric_col_type one. I am not sure if that's what you want there. Even if you do, looks like default should be numeric, not string.
Adding a basic new bigquery surfacer which will insert metrics in the bigquery table. To make it more efficient, we're using batch insertion with a batch size of 1000 and a default interval of 10 seconds(it is configurable and can be changed while defining surfacer).
It can take arguments like project name, bigquery dataset, bigquery table name, table columns name and type, batch insertion interval and bigquery timeout. At this point, it only supports data type string, int/integer, float/double and timestamp. For timestamp, the user will have to pass epoch time (in milliseconds). In future, we can add support for more data types.