client/internal/logdog/butler/output/gcps/doc.go - Issue 1211053004: LogDog: Add Butler Output package.

Side by Side Diff: client/internal/logdog/butler/output/gcps/doc.go

Issue 1211053004: LogDog: Add Butler Output package. (Closed) Base URL: https://github.com/luci/luci-go@logdog-review-streamserver

Patch Set: Added some doc.go's. Created 5 years, 1 month ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

« client/internal/logdog/butler/output/doc.go ('K') | « client/internal/logdog/butler/output/doc.go ('k') | client/internal/logdog/butler/output/gcps/gcpsOutput.go » ('j') | client/internal/logdog/butler/output/gcps/gcpsOutput.go » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 // Copyright 2015 The Chromium Authors. All rights reserved.

	2 // Use of this source code is governed by a BSD-style license that can be

	3 // found in the LICENSE file.

	4

	5 // Package gcps implements the "gcps" Output.

	6 //

	7 // The "gcps" (Google Cloud Pub/Sub) Output publishes ButlerLogBundle protobufs

	8 // to a Google Cloud Pub/Sub topic. Each published data chunk consists of two

	9 // RecordIO frames:
	estaab 2015/11/19 23:57:24 Generally we don't mix message types across record Generally we don't mix message types across records in recordio (at least the one I'm familiar with :)) and we'd achieve the same functionality with a message that encapsulates both ButlerMetadata and ButlerLogBundle. Then the recipient wouldn't need to understand recordio, either. What are the advantages to doing it with 2 records instead that I'm missing? estaab 2015/11/19 23:57:24 Generally we don't mix message types across record Generally we don't mix message types across records in recordio. Could the same functionality be achieved with a message that encapsulates both ButlerMetadata and ButlerLogBundle? Advantages of having all pubsub messages being proto instead of recordio: 1) sender and receiver only need to speak proto, not recordio 2) we have command line tools to print protos as ascii even without knowing the message type (you just get tag numbers, but still useful for figuring out what's going on) dnj 2015/11/20 01:55:12 ButlerLogBundle may be compressed. The compression Show quoted text On 2015/11/19 23:57:24, estaab wrote: > Generally we don't mix message types across records in recordio (at least the > one I'm familiar with :)) and we'd achieve the same functionality with a message > that encapsulates both ButlerMetadata and ButlerLogBundle. Then the recipient > wouldn't need to understand recordio, either. What are the advantages to doing > it with 2 records instead that I'm missing? ButlerLogBundle may be compressed. The compression is defined in ButlerMetadata, so the former must be in a known format and the latter's format is defined by the former. Now, I could embed ButlerLogBundle as a "Data []byte" field in ButlerMetadata, but if I do that, the user has to read the full stream and allocate/copy the bytes once on ButlerMetadata deserialization and then again while actually processing. With this layout, a client can read this as a stream. The implicit protocol is "there is one RecordIO frame, and it is a ButlerMetadata". The ButlerMetadata describes the remainder of the stream, which is "there is another ReceordIO frame, and it is a ButlerLogBundle compressed/etc.". estaab 2015/11/20 04:37:27 Pubsub messages already get gzip-compressed at the Show quoted text On 2015/11/20 01:55:12, dnj wrote: > On 2015/11/19 23:57:24, estaab wrote: > > Generally we don't mix message types across records in recordio (at least the > > one I'm familiar with :)) and we'd achieve the same functionality with a > message > > that encapsulates both ButlerMetadata and ButlerLogBundle. Then the recipient > > wouldn't need to understand recordio, either. What are the advantages to doing > > it with 2 records instead that I'm missing? > > ButlerLogBundle may be compressed. The compression is defined in ButlerMetadata, > so the former must be in a known format and the latter's format is defined by > the former. > > Now, I could embed ButlerLogBundle as a "Data []byte" field in ButlerMetadata, > but if I do that, the user has to read the full stream and allocate/copy the > bytes once on ButlerMetadata deserialization and then again while actually > processing. > > With this layout, a client can read this as a stream. The implicit protocol is > "there is one RecordIO frame, and it is a ButlerMetadata". The ButlerMetadata > describes the remainder of the stream, which is "there is another ReceordIO > frame, and it is a ButlerLogBundle compressed/etc.". Pubsub messages already get gzip-compressed at the HTTP level. Why do we need to complicate this code by doing it at the application level also? HTTP client created in pubsub.NewClient: https://github.com/GoogleCloudPlatform/gcloud-golang/blob/master/pubsub/pubsu... which in turn calls transport.NewHTTPClient: https://github.com/GoogleCloudPlatform/gcloud-golang/blob/master/internal/tra... Which in turn calls oauth2.NewClient, and DisableCompression isn't getting set anywhere I can find down the call stack. And any competent library performing RPCs should be compressing messages anyway - I only dug into the implementation just to confirm it. dnj (Google) 2015/11/20 04:45:48 PubSub enforces a maximum message size, evaluating Show quoted text > Pubsub messages already get gzip-compressed at the HTTP level. Why do we need to > complicate this code by doing it at the application level also? PubSub enforces a maximum message size, evaluating the raw Message data size (regardless of HTTP compression). This lets us pack more into that space. estaab 2015/11/20 05:15:48 Ok, I buy that argument. Am I looking at the right Show quoted text On 2015/11/20 04:45:48, dnj (Google) wrote: > > Pubsub messages already get gzip-compressed at the HTTP level. Why do we need > to > > complicate this code by doing it at the application level also? > > PubSub enforces a maximum message size, evaluating the raw Message data size > (regardless of HTTP compression). This lets us pack more into that space. Ok, I buy that argument. Am I looking at the right place where I see requests can be up to 10MB? Is that a limit we expect to approach? Looking here: https://cloud.google.com/pubsub/quotas If the need for the extra complexity isn't already documented somewhere can you add a note? dnj (Google) 2015/11/20 16:40:02 Not really expect to approach, but it depends. Mos Show quoted text On 2015/11/20 05:15:48, estaab wrote: > On 2015/11/20 04:45:48, dnj (Google) wrote: > > > Pubsub messages already get gzip-compressed at the HTTP level. Why do we > need > > to > > > complicate this code by doing it at the application level also? > > > > PubSub enforces a maximum message size, evaluating the raw Message data size > > (regardless of HTTP compression). This lets us pack more into that space. > > Ok, I buy that argument. Am I looking at the right place where I see requests > can be up to 10MB? Is that a limit we expect to approach? > Looking here: https://cloud.google.com/pubsub/quotas Not really expect to approach, but it depends. Most builds will send more than 10MB of logs. The remainder is then buffering/timeout configuration. I don't think we'll tune to retain 10MB and burst it, but this implementation doesn't make those decisions. In this case I'm baking wire versatility into the protocol even if it's not strictly necessary on rollout day. This makes it so things like buffering parameters are tunable without rebooting the entire stack. Show quoted text > If the need for the extra complexity isn't already documented somewhere can you > add a note? This whole thing is actually documented in detail in ./common/logdog/butlerproto. It's just restated here for reference. I'll add a note to this effect.
	10 // Frame #0) A ButlerMetadata protobuf which describes Frame #1.

	11 // Frame #1) A ButlerLogBundle protobuf containing the bundle data.

	12 package gcps

OLD	NEW