Skip to content

Commit

Permalink
util/log: only compute content-type once
Browse files Browse the repository at this point in the history
This is a fixup to cockroachdb#77014:

- the content-type is not a property of the format *parser*, it's a
  property of the format itself. Touching `formatParsers` to compute
  it is an abstraction mismatch.

- the content-type is constant for a given log config. There's no
  need to re-compute it on every log entry. It can be computed
  and stored once when the config is applied in `flags.go`.

  (Also, the `outputLogEntry` code is time-sensitive, so we don't want
  to give it more work.)

- the content-type is only needed for HTTP sinks. No need
  to compute it for other sink types. So its retrieval can be
  confined to `newHTTPSink`.

Release justification: bug fix

Release note: None
  • Loading branch information
knz authored and rimadeodhar committed Mar 3, 2022
1 parent cb57f8c commit bc5adb5
Show file tree
Hide file tree
Showing 9 changed files with 44 additions and 21 deletions.
1 change: 0 additions & 1 deletion pkg/util/log/clog.go
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,6 @@ func (l *loggerT) outputLogEntry(entry logEntry) {
// The sink was not accepting entries at this level. Nothing to do.
continue
}

if err := s.sink.output(extraFlush, bufs.b[i].Bytes()); err != nil {
if !s.criticality {
// An error on this sink is not critical. Just report
Expand Down
1 change: 1 addition & 0 deletions pkg/util/log/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,7 @@ func newHTTPSinkInfo(c logconfig.HTTPSinkConfig) (*sinkInfo, error) {
unsafeTLS: *c.UnsafeTLS,
timeout: *c.Timeout,
disableKeepAlives: *c.DisableKeepAlives,
contentType: info.formatter.contentType(),
})
if err != nil {
return nil, err
Expand Down
8 changes: 8 additions & 0 deletions pkg/util/log/format_crdb_v1.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ func (formatCrdbV1) formatEntry(entry logEntry) *buffer {

func (formatCrdbV1) doc() string { return formatCrdbV1CommonDoc(false /* withCounter */) }

func (formatCrdbV1) contentType() string { return "text/plain" }

func formatCrdbV1CommonDoc(withCounter bool) string {
var buf strings.Builder

Expand Down Expand Up @@ -167,6 +169,8 @@ func (formatCrdbV1WithCounter) formatEntry(entry logEntry) *buffer {

func (formatCrdbV1WithCounter) doc() string { return formatCrdbV1CommonDoc(true /* withCounter */) }

func (formatCrdbV1WithCounter) contentType() string { return "text/plain" }

// formatCrdbV1TTY is like formatCrdbV1 and includes VT color codes if
// the stderr output is a TTY and -nocolor is not passed on the
// command line.
Expand All @@ -192,6 +196,8 @@ func (formatCrdbV1TTY) doc() string {
return "Same textual format as `" + formatCrdbV1{}.formatterName() + "`." + ttyFormatDoc
}

func (formatCrdbV1TTY) contentType() string { return "text/plain" }

// formatCrdbV1ColorWithCounter is like formatCrdbV1WithCounter and
// includes VT color codes if the stderr output is a TTY and -nocolor
// is not passed on the command line.
Expand All @@ -211,6 +217,8 @@ func (formatCrdbV1TTYWithCounter) doc() string {
return "Same textual format as `" + formatCrdbV1WithCounter{}.formatterName() + "`." + ttyFormatDoc
}

func (formatCrdbV1TTYWithCounter) contentType() string { return "text/plain" }

// formatEntryInternalV1 renders a log entry.
// Log lines are colorized depending on severity.
// It uses a newly allocated *buffer. The caller is responsible
Expand Down
4 changes: 4 additions & 0 deletions pkg/util/log/format_crdb_v2.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ func (formatCrdbV2) formatEntry(entry logEntry) *buffer {

func (formatCrdbV2) doc() string { return formatCrdbV2CommonDoc() }

func (formatCrdbV2) contentType() string { return "text/plain" }

func formatCrdbV2CommonDoc() string {
var buf strings.Builder

Expand Down Expand Up @@ -188,6 +190,8 @@ func (formatCrdbV2TTY) doc() string {
return "Same textual format as `" + formatCrdbV2{}.formatterName() + "`." + ttyFormatDoc
}

func (formatCrdbV2TTY) contentType() string { return "text/plain" }

// formatEntryInternalV2 renders a log entry.
// Log lines are colorized depending on severity.
// It uses a newly allocated *buffer. The caller is responsible
Expand Down
8 changes: 8 additions & 0 deletions pkg/util/log/format_json.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ func (f formatFluentJSONCompact) formatEntry(entry logEntry) *buffer {
return formatJSON(entry, true /* fluent */, tagCompact)
}

func (formatFluentJSONCompact) contentType() string { return "application/json" }

type formatFluentJSONFull struct{}

func (formatFluentJSONFull) formatterName() string { return "json-fluent" }
Expand All @@ -41,6 +43,8 @@ func (f formatFluentJSONFull) formatEntry(entry logEntry) *buffer {

func (formatFluentJSONFull) doc() string { return formatJSONDoc(true /* fluent */, tagVerbose) }

func (formatFluentJSONFull) contentType() string { return "application/json" }

type formatJSONCompact struct{}

func (formatJSONCompact) formatterName() string { return "json-compact" }
Expand All @@ -51,6 +55,8 @@ func (f formatJSONCompact) formatEntry(entry logEntry) *buffer {

func (formatJSONCompact) doc() string { return formatJSONDoc(false /* fluent */, tagCompact) }

func (formatJSONCompact) contentType() string { return "application/json" }

type formatJSONFull struct{}

func (formatJSONFull) formatterName() string { return "json" }
Expand All @@ -61,6 +67,8 @@ func (f formatJSONFull) formatEntry(entry logEntry) *buffer {

func (formatJSONFull) doc() string { return formatJSONDoc(false /* fluent */, tagVerbose) }

func (formatJSONFull) contentType() string { return "application/json" }

func formatJSONDoc(forFluent bool, tags tagChoice) string {
var buf strings.Builder
buf.WriteString(`This format emits log entries as a JSON payload.
Expand Down
4 changes: 4 additions & 0 deletions pkg/util/log/formats.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ type logFormatter interface {
// formatEntry formats a logEntry into a newly allocated *buffer.
// The caller is responsible for calling putBuffer() afterwards.
formatEntry(entry logEntry) *buffer

// contentType is the MIME content-type field to use on
// transports which use this metadata.
contentType() string
}

var formatParsers = map[string]string{
Expand Down
32 changes: 15 additions & 17 deletions pkg/util/log/http_sink.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,7 @@ type httpSinkOptions struct {
timeout time.Duration
method string
disableKeepAlives bool
}

// formatToContentType map contains a mapping from the log format
// to the content type header that should be set for that format
// for the HTTP POST method. The content type header defaults
// to `text/plain` when the http sink is configured with a format
// not included in this map.
var formatToContentType = map[string]string{
"json": "application/json",
contentType string
}

func newHTTPSink(url string, opt httpSinkOptions) (*httpSink, error) {
Expand All @@ -53,8 +45,9 @@ func newHTTPSink(url string, opt httpSinkOptions) (*httpSink, error) {
Transport: transport,
Timeout: opt.timeout,
},
address: url,
doRequest: doPost,
address: url,
doRequest: doPost,
contentType: "application/octet-stream",
}

if opt.unsafeTLS {
Expand All @@ -65,13 +58,18 @@ func newHTTPSink(url string, opt httpSinkOptions) (*httpSink, error) {
hs.doRequest = doGet
}

if opt.contentType != "" {
hs.contentType = opt.contentType
}

return hs, nil
}

type httpSink struct {
client http.Client
address string
doRequest func(sink *httpSink, logEntry []byte) (*http.Response, error)
client http.Client
address string
contentType string
doRequest func(sink *httpSink, logEntry []byte) (*http.Response, error)
}

// output emits some formatted bytes to this sink.
Expand All @@ -85,15 +83,15 @@ type httpSink struct {

func (hs *httpSink) output(extraSync bool, b []byte) (err error) {
resp, err := hs.doRequest(hs, b)

if err != nil {
return err
}

if resp.StatusCode >= 400 {
return HTTPLogError{
StatusCode: resp.StatusCode,
Address: hs.address}
Address: hs.address,
}
}
return nil
}
Expand All @@ -109,7 +107,7 @@ func (hs *httpSink) emergencyOutput(b []byte) {
}

func doPost(hs *httpSink, b []byte) (*http.Response, error) {
resp, err := hs.client.Post(hs.address, "text/plain", bytes.NewReader(b))
resp, err := hs.client.Post(hs.address, hs.contentType, bytes.NewReader(b))
if err != nil {
return nil, err
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/util/log/http_sink_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ func TestHTTPSinkContentTypeJSON(t *testing.T) {
address := "http://localhost" // testBase appends the port
timeout := 5 * time.Second
tb := true
format := "json"
format := "json-fluent"
expectedContentType := "application/json"
defaults := logconfig.HTTPDefaults{
Address: &address,
Expand All @@ -232,7 +232,7 @@ func TestHTTPSinkContentTypeJSON(t *testing.T) {
t.Log(body)
contentType := header.Get("Content-Type")
if contentType != expectedContentType {
return errors.Newf("mismatched content type: expected %s, got %s")
return errors.Newf("mismatched content type: expected %s, got %s", expectedContentType, contentType)
}
return nil
}
Expand Down Expand Up @@ -266,7 +266,7 @@ func TestHTTPSinkContentTypePlainText(t *testing.T) {
t.Log(body)
contentType := header.Get("Content-Type")
if contentType != expectedContentType {
return errors.Newf("mismatched content type: expected %s, got %s")
return errors.Newf("mismatched content type: expected %s, got %s", expectedContentType, contentType)
}
return nil
}
Expand Down
1 change: 1 addition & 0 deletions pkg/util/log/intercept.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ type formatInterceptor struct{}

func (formatInterceptor) formatterName() string { return "json-intercept" }
func (formatInterceptor) doc() string { return "internal only" }
func (formatInterceptor) contentType() string { return "application/json" }
func (formatInterceptor) formatEntry(entry logEntry) *buffer {
pEntry := entry.convertToLegacy()
buf := getBuffer()
Expand Down

0 comments on commit bc5adb5

Please sign in to comment.