Update "eventupload" library for client usage. |
|||||
Issue description
Kitchen wants to use "eventupload" to upload build events. Before it can, the "eventupload" library needs a few changes to make it run in a suitable environment:
1) We need a way to supply client options to the bigquery.Client used by the Uploader. Maybe add []option.ClientOption to UploaderConfig?
2) I'd like to not call the Metadata RPC on instantiation. Recommend either:
2a) Infer and retain Schema using "bigquery.InferSchema" from the first element in the first "Put" call.
2b) Specify an exemplar struct on creation and use "bigquery.InferSchema" there (e.g., parameter exampleIface interface{}).
3) Put should accept a Context, so individual Put calls can supply their own Context constraints.
3a) Similarly, we don't need to retain a Context if we just pass it at Put-time, which is a win.
3b) Also don't need to set "timeout", since if we supply a Context, the caller can apply a timeout themselves if they wish to have one.
4) We need to be able to upload pointer-to-struct. Currently, the reflection only allows or slices of struct or by-value structs.
4a) Maybe reduce complexity and not accept slices? e.g., "Put(ctx context.Context, events ...interface{})"
Marking P1 b/c these changes are needed by Kitchen event monitoring. WDYT?
,
Aug 18 2017
- Migrate tests to "goconvey" (Infra and LUCI use this as a standard testing platform).
,
Aug 18 2017
Why do you prefer to not call the Metadata RPC on instantiation?
,
Aug 18 2017
RE #3, it's an unnecessary round-trip RPC. We can infer the schema locally from the struct that we're ingesting, I think?
,
Aug 18 2017
,
Aug 21 2017
not making an RPC is better than making because it is faster at least marking Assigned, because the bug has an Owner
,
Aug 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/infra/infra/+/0163b33ba9a79cb55f05f3f1ccb32c9755cb110a commit 0163b33ba9a79cb55f05f3f1ccb32c9755cb110a Author: Dan Jacques <dnj@google.com> Date: Thu Aug 24 18:37:58 2017 [eventupload] Remove NewUploader error value. After some recent patches, the error value is no longer used. Remove it to simplify the API! BUG= chromium:757060 TEST=None R=katthomas@chromium.org Change-Id: I63f7d4e1bea96a92b797f25e26f7fe2b19c76abc Reviewed-on: https://chromium-review.googlesource.com/633912 Reviewed-by: Katie Thomas <katthomas@chromium.org> Commit-Queue: Daniel Jacques <dnj@chromium.org> [modify] https://crrev.com/0163b33ba9a79cb55f05f3f1ccb32c9755cb110a/go/src/infra/libs/eventupload/eventupload.go
,
Sep 1 2017
I forgot to tag this but on the reviews, but eventupload is ready for client use! In fact, it is being used by kitchen in production already. There are still a few nice-to-haves: Modify eventuploader to use luci/common/logging for logging Convert tests to go convey Expose the bigquery.Uploader directly so we don't need to duplicate its options in UploaderConfig But these are lower priority and not blocking client usage, so I'm going to bump this to P2.
,
Sep 1 2017
SGTM
,
Sep 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/infra/infra/+/590153708132fe2540ab1ae9ff035ce9bbf72fc2 commit 590153708132fe2540ab1ae9ff035ce9bbf72fc2 Author: Katie Thomas <katthomas@google.com> Date: Fri Sep 01 22:23:59 2017 Use luci/common/logging in eventupload Bug:757060 Change-Id: I6e7d0fee06bf982837452f8f7de974c68ce8e9a6 Reviewed-on: https://chromium-review.googlesource.com/647380 Reviewed-by: Nodir Turakulov <nodir@chromium.org> Commit-Queue: Katie Thomas <katthomas@google.com> [modify] https://crrev.com/590153708132fe2540ab1ae9ff035ce9bbf72fc2/go/src/infra/libs/eventupload/insertid_test.go [modify] https://crrev.com/590153708132fe2540ab1ae9ff035ce9bbf72fc2/go/src/infra/libs/eventupload/insertid.go [modify] https://crrev.com/590153708132fe2540ab1ae9ff035ce9bbf72fc2/go/src/infra/libs/eventupload/eventupload.go
,
Sep 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/infra/infra/+/ad95c01977e706ace090aa3c6613698a2d287211 commit ad95c01977e706ace090aa3c6613698a2d287211 Author: Katie Thomas <katthomas@google.com> Date: Tue Sep 05 23:37:36 2017 Use Convey for eventupload tests Bug:757060 Change-Id: I54877679a16f1d637a09008c7260493b81fba94f Reviewed-on: https://chromium-review.googlesource.com/648364 Reviewed-by: Daniel Jacques <dnj@chromium.org> Commit-Queue: Katie Thomas <katthomas@google.com> [modify] https://crrev.com/ad95c01977e706ace090aa3c6613698a2d287211/go/src/infra/libs/eventupload/insertid_test.go [modify] https://crrev.com/ad95c01977e706ace090aa3c6613698a2d287211/go/src/infra/libs/eventupload/eventupload_test.go
,
Sep 7 2017
,
Sep 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/infra/infra/+/b918aad70d21b0d28377fa2a1560e850c44f5dce commit b918aad70d21b0d28377fa2a1560e850c44f5dce Author: Katie Thomas <katthomas@google.com> Date: Thu Sep 07 17:48:09 2017 Expose bigquery.Uploader Bug:757060 Change-Id: I9d4eef9e91619f5579117fbd7081d8b68121598d Reviewed-on: https://chromium-review.googlesource.com/650512 Reviewed-by: Daniel Jacques <dnj@chromium.org> Commit-Queue: Katie Thomas <katthomas@google.com> [modify] https://crrev.com/b918aad70d21b0d28377fa2a1560e850c44f5dce/go/src/infra/tools/kitchen/monitoring.go [modify] https://crrev.com/b918aad70d21b0d28377fa2a1560e850c44f5dce/go/src/infra/libs/eventupload/eventupload.go
,
Sep 7 2017
thanks!
,
Sep 7 2017
Awesome, thanks for doing this! |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by d...@chromium.org
, Aug 18 2017