Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Switch Thrift with Jaeger's fork #3050

Merged
merged 4 commits into from
Jun 3, 2021
Merged

Switch Thrift with Jaeger's fork #3050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
056942b
Switched Thrift with Jaeger's fork
jpkrohling Jun 3, 2021
af78a7e
Changed assertion on error
jpkrohling Jun 3, 2021
700c95d
Changes based on the review
jpkrohling Jun 3, 2021
61d1790
Merge branch 'master' into jpkrohling/patched-thrift
yurishkuro Jun 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/collector/app/zipkin/http_handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ func TestFormatBadBody(t *testing.T) {
statusCode, resBodyStr, err := postBytes(server.URL+`/api/v1/spans`, []byte("not good"), createHeader("application/x-thrift"))
assert.NoError(t, err)
assert.EqualValues(t, http.StatusBadRequest, statusCode)
assert.EqualValues(t, "Unable to process request body: Unknown data type 111\n", resBodyStr)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was interesting, as the new error is this: Unable to process request body: size exceeded max allowed: 1869881447. This seems to indicate that the fix is being picked up, but it did raise a warning in my head: are we really allocating more than 1GiB for this? Apparently, no. I used runtime.MemStats to measure the memory consumption before and after the HTTP call, and the usage was minimal (2 Alloc vs. 3 Alloc).

Copy link

@fishy fishy Jun 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is indeed interesting.

The old error message "Unknown data type" comes from Skip function, which is called when either the field id is not defined in the struct, or when the type of the field id doesn't match what's defined in the struct.

The new error message "size exceeded max allowed" comes from size sanity checks, which could be called by a lot of TProtocol functions (for example, ReadString, ReadBinary, ReadMapBegin, ReadListBegin, ReadSetBegin). This means the new code actually passed the field type check and is no longer skipped (or maybe previously the first field happened to match and it's the second field failed the field type check and caused the error, while in the new code the first field passed field type check but failed the size sanity check).

runtime.MemStats might be misleading. I believe it's the same as benchmark tests' ReportAllocs, and this benchmark test reported 0 allocs/0 bytes which is certainly a lie (I have to use a much smaller size because the original one is too slow):

package main

import (
        "testing"
)

// const size = 1869881447
const size = 1869

func BenchmarkAlloc(b *testing.B) {
        b.ReportAllocs()
        for i := 0; i < b.N; i++ {
                _ = make([]int, size)
        }
}
$ go test -bench . -benchmem
goos: linux
goarch: amd64
pkg: foo
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkAlloc-12       1000000000               0.2487 ns/op          0 B/op          0 allocs/op
PASS
ok      foo     0.278s

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fishy, do you think this change raises flags? I wouldn't expect the payload for this test to cause such a huge message.

Copy link

@fishy fishy Jun 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think this change raises flags?

Not necessarily.

If you dig into the code on what this does, the first ever read from the thrift payload is to ReadListBegin, which is one of the functions that would trigger the new error.

Let's first convert the payload ([]byte("not good")) into raw bytes: [6e 6f 74 20 67 6f 6f 64].

In ReadListBegin implementation, first it reads a byte, 6e, then it tries to read an int32 as the size of the list. For the reading of the int32, it reads the next 4 bytes (6f 74 20 67), decode with bigendian, which results in 1869881447.

So this is totally expected behavior.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And following ReadListBegin, jaeger's code didn't really do the pre-allocation. it just append spans to the slice and rely on append to do the allocation and grow the slice.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

y'all even already have a comment on that :)

jaeger/model/converter/thrift/zipkin/deserialize.go

Lines 51 to 52 in ff32436

// We don't depend on the size returned by ReadListBegin to preallocate the array because it
// sometimes returns a nil error on bad input and provides an unreasonably large int for size

assert.Contains(t, resBodyStr, "Unable to process request body:")
}

func TestCannotReadBodyFromRequest(t *testing.T) {
Expand Down
5 changes: 3 additions & 2 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ require (
github.com/opentracing-contrib/go-stdlib v0.0.0-20190519235532-cf7a6c988dc9
github.com/opentracing/opentracing-go v1.2.0
github.com/prometheus/client_golang v1.5.1
github.com/prometheus/common v0.10.0 // indirect
github.com/prometheus/common v0.10.0
github.com/prometheus/procfs v0.1.3 // indirect
github.com/rs/cors v1.7.0
github.com/securego/gosec v0.0.0-20200203094520-d13bb6d2420c
Expand Down Expand Up @@ -73,4 +73,5 @@ require (
honnef.co/go/tools v0.2.0
)

replace github.com/gogo/protobuf => github.com/gogo/protobuf v1.3.2
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The protobuf dependency has been removed, as the one used in the main section is the same.

// TODO: remove once the 0.14.2 or 0.15.0 is released
replace github.com/apache/thrift => github.com/jaegertracing/thrift v0.14.1-patch1
jpkrohling marked this conversation as resolved.
Show resolved Hide resolved
11 changes: 7 additions & 4 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,6 @@ github.com/alecthomas/template v0.0.0-20190718012654-fb15b899a751/go.mod h1:LOuy
github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0=
github.com/alecthomas/units v0.0.0-20190717042225-c3de453c63f4/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0=
github.com/andreyvit/diff v0.0.0-20170406064948-c7f18ee00883/go.mod h1:rCTlJbsFo29Kk6CurOXKm700vrz8f0KW0JNfpkRJY/8=
github.com/apache/thrift v0.12.0/go.mod h1:cp2SuWMxlEZw2r+iP2GNCdIi4C1qmUzdZFSVb+bacwQ=
github.com/apache/thrift v0.13.0/go.mod h1:cp2SuWMxlEZw2r+iP2GNCdIi4C1qmUzdZFSVb+bacwQ=
github.com/apache/thrift v0.14.1 h1:Yh8v0hpCj63p5edXOLaqTJW0IJ1p+eMW6+YSOqw1d6s=
github.com/apache/thrift v0.14.1/go.mod h1:cp2SuWMxlEZw2r+iP2GNCdIi4C1qmUzdZFSVb+bacwQ=
github.com/armon/circbuf v0.0.0-20150827004946-bbbad097214e/go.mod h1:3U/XgcO3hCbHZ8TKRvWD2dDTCfh9M9ya+I9JpbB7O8o=
github.com/armon/consul-api v0.0.0-20180202201655-eb2c6b5be1b6/go.mod h1:grANhF5doyWs3UAsr3K4I6qtAmlQcZDesFNEHPZAzj8=
github.com/armon/go-metrics v0.0.0-20180917152333-f0300d1749da/go.mod h1:Q73ZrmVTwzkszR9V5SSuryQ31EELlFMUz1kKyl939pY=
Expand Down Expand Up @@ -286,6 +282,9 @@ github.com/gocql/gocql v0.0.0-20200228163523-cd4b606dd2fb/go.mod h1:DL0ekTmBSTdl
github.com/gogo/googleapis v1.1.0/go.mod h1:gf4bu3Q80BeJ6H1S1vYPm8/ELATdvryBaNFGgqEef3s=
github.com/gogo/googleapis v1.4.1 h1:1Yx4Myt7BxzvUr5ldGSbwYiZG6t9wGBZ+8/fX3Wvtq0=
github.com/gogo/googleapis v1.4.1/go.mod h1:2lpHqI5OcWCtVElxXnPt+s8oJvMpySlOyM6xDCrzib4=
github.com/gogo/protobuf v1.1.1/go.mod h1:r8qH/GZQm5c6nD/R0oafs1akxWv10x8SbQlK7atdtwQ=
github.com/gogo/protobuf v1.2.0/go.mod h1:r8qH/GZQm5c6nD/R0oafs1akxWv10x8SbQlK7atdtwQ=
github.com/gogo/protobuf v1.2.1/go.mod h1:hp+jE20tsWTFYpLwKvXlhS1hjn+gTNwPg2I6zVXpSg4=
github.com/gogo/protobuf v1.3.2 h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q=
github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69NZV8Q=
github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b/go.mod h1:SBH7ygxi8pfUlaOkMMuAQtPIUF8ecWP5IEl/CR7VP2Q=
Expand Down Expand Up @@ -392,6 +391,8 @@ github.com/hudl/fargo v1.3.0/go.mod h1:y3CKSmjA+wD2gak7sUSXTAoopbhU08POFhmITJgmK
github.com/inconshreveable/mousetrap v1.0.0 h1:Z8tu5sraLXCXIcARxBp/8cbvlwVa7Z1NHg9XEKhtSvM=
github.com/inconshreveable/mousetrap v1.0.0/go.mod h1:PxqpIevigyE2G7u3NXJIT2ANytuPF1OarO4DADm73n8=
github.com/influxdata/influxdb1-client v0.0.0-20191209144304-8bf82d3c094d/go.mod h1:qj24IKcXYK6Iy9ceXlo3Tc+vtHo9lIhSX5JddghvEPo=
github.com/jaegertracing/thrift v0.14.1-patch1 h1:NcAyet3G3NmmxE9zNrUcdQIeDU7NBP0M4Hsigw28KPQ=
github.com/jaegertracing/thrift v0.14.1-patch1/go.mod h1:OdHPxgcEtN+TatW0e/SZ6NA1AwyMikWYQYYsX6AAiEY=
github.com/jcmturner/aescts/v2 v2.0.0 h1:9YKLH6ey7H4eDBXW8khjYslgyqG2xZikXP0EQFKrle8=
github.com/jcmturner/aescts/v2 v2.0.0/go.mod h1:AiaICIRyfYg35RUkr8yESTqvSy7csK90qZ5xfvvsoNs=
github.com/jcmturner/dnsutils/v2 v2.0.0 h1:lltnkeZGL0wILNvrNiVCR6Ro5PGU/SeBvVO/8c/iPbo=
Expand Down Expand Up @@ -424,6 +425,7 @@ github.com/jtolds/gls v4.20.0+incompatible/go.mod h1:QJZ7F/aHp+rZTRtaJ1ow/lLfFfV
github.com/julienschmidt/httprouter v1.2.0/go.mod h1:SYymIcj16QtmaHHD7aYtjjsJG7VTCxuUUipMqKk8s4w=
github.com/karrick/godirwalk v1.8.0/go.mod h1:H5KPZjojv4lE+QYImBI8xVtrBRgYrIVsaRPx4tDPEn4=
github.com/karrick/godirwalk v1.10.3/go.mod h1:RoGL9dQei4vP9ilrpETWE8CLOZ1kiN0LhBygSwrAsHA=
github.com/kisielk/errcheck v1.1.0/go.mod h1:EZBBE59ingxPouuu3KfxchcWSUPOHkagtvWXihfKN4Q=
github.com/kisielk/errcheck v1.2.0/go.mod h1:/BMXB+zMLi60iA8Vv6Ksmxu/1UDYcXs4uQLJ+jE2L00=
github.com/kisielk/errcheck v1.5.0/go.mod h1:pFxgyoBC7bSaBwPgfKdkLd5X25qrDl4LWUI2bnpBCr8=
github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck=
Expand Down Expand Up @@ -878,6 +880,7 @@ golang.org/x/time v0.0.0-20180412165947-fbb02b2291d2/go.mod h1:tRJNPiyCQ0inRvYxb
golang.org/x/time v0.0.0-20181108054448-85acf8d2951c/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
golang.org/x/time v0.0.0-20190308202827-9d24e82272b4/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
golang.org/x/time v0.0.0-20191024005414-555d28b269f0/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
golang.org/x/tools v0.0.0-20180221164845-07fd8470d635/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
golang.org/x/tools v0.0.0-20180828015842-6cd1fcedba52/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
golang.org/x/tools v0.0.0-20181030221726-6c7e314b6563/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
Expand Down