Implement tracing package for toolkit #193

burov · 2020-06-13T22:06:14Z

Refactoring for SpanContextAnnotator
Implement wrapper to extend default http plugin
Implement wrapper to extend default gRPC plugin
Util functions CreateSpan, GetCurrent, AddLogs, AddAttributes
Builder for exporter
Add grpc error into span if exists
Obfuscation for headers with sensitive data
Automated tests

SpanContextAnnotator retrieve current trace.Span from context.Context or, in case there is no open trace.Span assemble trace.SpanContext from HTTP headers

* Add options to configure what should be added to span * Implement propogation of request/response headers, and body to span

Evgeniy-L · 2020-06-14T19:14:37Z

tracing/http.go

+	ReducedMarkerKey = "reduced_payload"
+
+	//ReducedMarkerValue is a value for annotation which will be presented in span in case payload was reduced
+	ReducedMarkerValue = "true"


is it a good idea to use "true" string here? It is something we invented or values is forced by 3rd party?

It our own marker which we add to span with reduced body, to allow developers to filter out them, if they want to see spans only with full body, for me "true" good, any idea how to do it more descriptive ?

I was thinking about using "reduced" or "enabled" string instead of "true". I am fine with any option (including existing), so it is up to you.

Actually we can discuss it, is it usefull to add some marker that span contains payload and signal not only that payload reduced but signal that payload actually exists for example payload.type = reduced|enabled allow developers to have more granural filtering, we will think about it

Are we compressing the payload ("reduced"), truncating (simply cut off after X characters and inject an ellipse to visually indicate truncation), or replacing the data (replacing data with a marker indicating that data has been replaced)

That a good point @mphillips-infoblox, i will change reduced to truncated actually, I already put "..." in the body but label for me seems reasonable to add annotation as well, in case somebody for example what to filterout traces without full body

Yes, as @Evgeniy-L suggested, if you are consistent and use something like payload.truncated = true, that would work really well

Evgeniy-L · 2020-06-14T19:15:50Z

tracing/http.go

+	DefaultMaxPayloadSize = 1024 * 1024
+
+	//ReducedMarkerKey is a key for annotation which will be presented in span in case payload was reduced
+	ReducedMarkerKey = "reduced_payload"


does it make same to have this key consistent to keys above ("." vs "_"). Maybe use payload.reduced.
It is something we invented or key is forced by 3rd party?

Thank you, that a good idea

tracing/http.go

Evgeniy-L · 2020-06-14T19:21:19Z

Gopkg.toml

@@ -38,6 +38,10 @@
  name = "google.golang.org/grpc"
  version = ">=1.0.0"

+[[constraint]]
+   name = "go.opencensus.io"
+   version = "0.22.3"


please use >= for version

Co-authored-by: Evgeniy Litvin <[email protected]>

drewwells

Are the UnaryServerInterceptor and StreamingServerInterceptor next?

drewwells · 2020-06-15T16:34:07Z

tracing/http.go

+}
+
+//NewTracingMiddleware wrap handler
+func NewTracingMiddleware(ops ...Option) func(http.Handler) http.Handler {


http middleware could be useful. This func is part of the tracing package, the name doesn't need to include tracing. Perhaps Middleware is a better name, implementers use tracing.Middleware

burov · 2020-06-15T17:13:22Z

Are the UnaryServerInterceptor and StreamingServerInterceptor next?

Actually no, we decided to leverage already existed plugins from open census and from our side just wrap them, and inreach them with additional functionality which important for us.

* Add options to configure what should be added to span * Implement propogation of inbound/outbound metadata, and payload to span

drewwells · 2020-06-16T16:52:08Z

tracing/http.go

+	options *httpOptions
+}
+
+func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {


What use case is this covering? If I make a grpc call to the service, it won't run this code. Wouldn't it be simpler to handle the gRPC case and not bother implementing the http Handler. We only need gateway to pass along headers to the underlying gRPC server

You are correct, however we want to be able to trace the call flow at all the protocol boundaries for a service. This allows us to not only see what the call properties are (headers, payload, etc) but we can also collect timing associated with the call pipeline for any entry point into/out of the service.

drewwells · 2020-06-16T16:58:45Z

tracing/grpc.go

+	}
+
+	return &ServerHandler{options: options}
+}


How do I read the span from the context.Context passed into middleware? For example, authZ reads a span from the passed context to its middleware then creates additional spans inside of it to trace OPA. See here: https://github.com/Infoblox-CTO/ngp.authz/pull/745/files#diff-fd5b75f59732938618e94d5a57a06c35R112

There are helper methods in this toolkit that take care of the plumbing for you. You just request a span passing in the context.

mphillips-infoblox · 2020-06-18T18:40:43Z

tracing/grpc.go

+}
+
+func obfuscate(x string) string {
+	countChars := int(float64(len(x)) * 0.20)


This should probably be a constant. We should consider making this configurable in the future.

The 0.20 value

mphillips-infoblox

Looking good to me

* Check is error not nil before annotate it with span * Get lattest span value after calling wrapped handler

Evgeniy-L

LGTM

* add resource identifiers Resource IDs are a typed guid proceeded by metadata helpful for humans to identify what the guid is referring to. stopping gRPC does not immediately guarantee http server is stopped. Serve() will call Stop() at the end. Stop() guarantees both servers are stopped and listeners are closed. * Expose HandleRequestID (#181) * Add an option to enable field mask override (#183) * Add an option to enable field mask override * Implement option pattern for PresenceClientInterceptor * Add WithOverrideFieldMask option to enable field mask override * Fix minor issues * provide test to override option * add test for multiple field masks * make requestid name consistent with JSON standart (#182) * update grpc * use >=1.4.1 for grpc-gateway * Add common logger with specific fields (#188) * Add common logger with specific fields * Add common logger, interceptors with customizable fields * Fix populating context * Fix unused arguments * Fix duplicate append ctx call, rename auxilary func name * Add custom fields from request headers * Add err handling if subject key is missing * Add err handling in case request_id missing * Begin transaction with options. (#189) * Begin transaction with options. * Extended `Transaction` object with support of begging transaction with options (to be able to set isolation level and RO option). Added `gorm.BeginTx()` wrapper; * Added analog wrapper to do same thing while extracting txn from context; * Updated UTs. * * Reduced code duplication in UTs. * Added comments. * Add stream interceptors (#190) * Add stream interceptors * Add UTs, fix subject field handling * Fix tests * Improve subject value handling * Remove redundant ctx returns, fix populating fields into cxt logger * Add notes to generate mocks * Add blank line * Logging interceptor refactor (#192) * use logrus server interceptors * lint auth package * move server stream mock outside of requestid for reuse * add stream server interceptor for account_id logging * Add FIXME on ClientInterceptor test When the test correctly supplies Outgoing metadata, AuthFromMD cannot extract the authorization header because it is designed for Incoming MD only. The GetAccountID() function will need to be rewritten to work in ClientInterceptors. * Implement tracing package for toolkit (#193) * add SpanContextAnnotator SpanContextAnnotator retrieve current trace.Span from context.Context or, in case there is no open trace.Span assemble trace.SpanContext from HTTP headers * Implement wrapper to extend ochttp.Handler * Add options to configure what should be added to span * Implement propogation of request/response headers, and body to span * Implement wrapper to extend ocgrpc.ServerHandler * Add options to configure what should be added to span * Implement propogation of inbound/outbound metadata, and payload to span * add util functions * add grpc error into span * implement simple exporter constructor * implement obfuscation logic * move obfuscation factor to constant * Get lattest span value after calling wrapped handler * Use library function to assemble span context * Fix request id key (#197) * fix request id key * fix formating * fix tests * add test whith deprecated request-id key * Provide customization to allow passing X-Geo-* headers into gRPC metadata (#201) * dep ensure * implement AtalsDefaultHeaderMatcher * use AtlasDefaultHeaderMatcher by default * Use AtlasDefaultHeaderMatcher in ExtendedHeaderMatcher * add ChainHeaderMatcher * Utilize ChainHeaderMatcher (#202) * Add unit tests (#203) * B-30111 - Add UTs * B-30111 - Add UTs * Add unit tests * 'unable to get field from token' spam message (#205) * 'unable to get field from token' spam message * fix TestHeadersToAttributes map * propagating context to gorm transaction (#207) * Migrate atlas toolkit to go mod (#208) migrated to go mod updated travis CI * Refactor Headers. Util to include Headers into outgoing context * Refactor Headers. Util to include Headers into outgoing context * Rename util funcs * Added support for graceful shutdown (#210) Added support for disabling automatic server stop * update Go deps with Dependabot (#209) * Bump github.com/sirupsen/logrus from 1.4.2 to 1.7.0 (#215) Bumps [github.com/sirupsen/logrus](https://github.com/sirupsen/logrus) from 1.4.2 to 1.7.0. - [Release notes](https://github.com/sirupsen/logrus/releases) - [Changelog](https://github.com/sirupsen/logrus/blob/master/CHANGELOG.md) - [Commits](sirupsen/logrus@v1.4.2...v1.7.0) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump github.com/golang/protobuf from 1.4.2 to 1.4.3 (#216) Bumps [github.com/golang/protobuf](https://github.com/golang/protobuf) from 1.4.2 to 1.4.3. - [Release notes](https://github.com/golang/protobuf/releases) - [Commits](golang/protobuf@v1.4.2...v1.4.3) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump golang from 1.10.0 to 1.15.8 in /tools/debugging (#212) Bumps golang from 1.10.0 to 1.15.8. Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump github.com/grpc-ecosystem/go-grpc-middleware from 1.2.0 to 1.2.2 (#217) Bumps [github.com/grpc-ecosystem/go-grpc-middleware](https://github.com/grpc-ecosystem/go-grpc-middleware) from 1.2.0 to 1.2.2. - [Release notes](https://github.com/grpc-ecosystem/go-grpc-middleware/releases) - [Changelog](https://github.com/grpc-ecosystem/go-grpc-middleware/blob/master/CHANGELOG.md) - [Commits](grpc-ecosystem/go-grpc-middleware@v1.2.0...v1.2.2) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump github.com/DATA-DOG/go-sqlmock from 1.4.1 to 1.5.0 (#218) Bumps [github.com/DATA-DOG/go-sqlmock](https://github.com/DATA-DOG/go-sqlmock) from 1.4.1 to 1.5.0. - [Release notes](https://github.com/DATA-DOG/go-sqlmock/releases) - [Commits](DATA-DOG/go-sqlmock@v1.4.1...v1.5.0) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump github.com/jinzhu/gorm from 1.9.12 to 1.9.16 (#213) Bumps [github.com/jinzhu/gorm](https://github.com/jinzhu/gorm) from 1.9.12 to 1.9.16. - [Release notes](https://github.com/jinzhu/gorm/releases) - [Commits](jinzhu/gorm@v1.9.12...v1.9.16) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump github.com/google/uuid from 1.1.1 to 1.2.0 (#219) Bumps [github.com/google/uuid](https://github.com/google/uuid) from 1.1.1 to 1.2.0. - [Release notes](https://github.com/google/uuid/releases) - [Commits](google/uuid@v1.1.1...v1.2.0) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump golang from 1.15.8 to 1.16.0 in /tools/debugging (#223) Bumps golang from 1.15.8 to 1.16.0. Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump github.com/sirupsen/logrus from 1.7.0 to 1.8.0 (#225) Bumps [github.com/sirupsen/logrus](https://github.com/sirupsen/logrus) from 1.7.0 to 1.8.0. - [Release notes](https://github.com/sirupsen/logrus/releases) - [Changelog](https://github.com/sirupsen/logrus/blob/master/CHANGELOG.md) - [Commits](sirupsen/logrus@v1.7.0...v1.8.0) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * valid token may not have subject field * Bump golang from 1.16.0 to 1.16.3 in /tools/debugging Bumps golang from 1.16.0 to 1.16.3. Signed-off-by: dependabot[bot] <[email protected]> * add OldStatusCreatedOnUpdate to support old grpc -> http status mapping * update comment. * fix fmt * Make interceptor accept custom transaction. Co-authored-by: Drew Wells <[email protected]> Co-authored-by: Arash Outadi <[email protected]> Co-authored-by: Aliaksei Burau <[email protected]> Co-authored-by: Daniel Garcia <[email protected]> Co-authored-by: Daniel Garcia <[email protected]> Co-authored-by: Anton Dudko <[email protected]> Co-authored-by: Maksim Volkov <[email protected]> Co-authored-by: Tom Hayward <[email protected]> Co-authored-by: Alexey Bynenkov <[email protected]> Co-authored-by: Pavel Rybintsev <[email protected]> Co-authored-by: Andrei Lavrov <[email protected]> Co-authored-by: Anton Dudko <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: binjip978 <[email protected]> Co-authored-by: Sergei Semenchuk <[email protected]>

add SpanContextAnnotator

d340d6d

SpanContextAnnotator retrieve current trace.Span from context.Context or, in case there is no open trace.Span assemble trace.SpanContext from HTTP headers

burov added WIP DONOTMERGE labels Jun 13, 2020

Implement wrapper to extend ochttp.Handler

b445c2e

* Add options to configure what should be added to span * Implement propogation of request/response headers, and body to span

Evgeniy-L self-requested a review June 14, 2020 19:09

Evgeniy-L reviewed Jun 14, 2020

View reviewed changes

burov requested a review from mphillips-infoblox June 15, 2020 08:02

burov and others added 4 commits June 15, 2020 11:07

Update tracing/http.go

3d8e501

Co-authored-by: Evgeniy Litvin <[email protected]>

Update tracing/http.go

5690e74

Co-authored-by: Evgeniy Litvin <[email protected]>

Update tracing/http.go

5c06926

Co-authored-by: Evgeniy Litvin <[email protected]>

address PR comments

ca008ba

drewwells reviewed Jun 15, 2020

View reviewed changes

burov added 2 commits June 16, 2020 14:11

Fix naming issues in http wrapper

e99506b

Implement wrapper to extend ocgrpc.ServerHandler

45fcbc6

* Add options to configure what should be added to span * Implement propogation of inbound/outbound metadata, and payload to span

drewwells reviewed Jun 16, 2020

View reviewed changes

burov added 5 commits June 17, 2020 17:35

add util functions

db0ce43

add grpc error into span

ce02ac9

Avoid adding sensitive headers to spans by def

cd51136

implement simple exporter constructor

b506390

implement obfuscation logic

593d22d

mphillips-infoblox reviewed Jun 18, 2020

View reviewed changes

mphillips-infoblox approved these changes Jun 18, 2020

View reviewed changes

burov added 3 commits June 18, 2020 22:04

fix typo

f34d4fb

move obfuscation factor to constant

e96a088

Smal fixes

89f9b42

* Check is error not nil before annotate it with span * Get lattest span value after calling wrapped handler

burov removed DONOTMERGE WIP labels Jun 19, 2020

burov requested a review from Evgeniy-L June 19, 2020 13:17

Evgeniy-L approved these changes Jun 19, 2020

View reviewed changes

Use library function to assemble span context

f5497a6

burov merged commit bdfb656 into infobloxopen:master Jun 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement tracing package for toolkit #193

Implement tracing package for toolkit #193

burov commented Jun 13, 2020 •

edited

Loading

Evgeniy-L Jun 14, 2020

burov Jun 15, 2020

Evgeniy-L Jun 15, 2020

burov Jun 15, 2020

mphillips-infoblox Jun 15, 2020

burov Jun 15, 2020

mphillips-infoblox Jun 15, 2020

Evgeniy-L Jun 14, 2020

burov Jun 15, 2020

Evgeniy-L Jun 14, 2020

burov Jun 15, 2020

drewwells left a comment

drewwells Jun 15, 2020

burov Jun 15, 2020

burov commented Jun 15, 2020

drewwells Jun 16, 2020

mphillips-infoblox Jun 16, 2020

drewwells Jun 16, 2020

mphillips-infoblox Jun 18, 2020

mphillips-infoblox Jun 18, 2020

mphillips-infoblox Jun 18, 2020

mphillips-infoblox left a comment

Evgeniy-L left a comment

Implement tracing package for toolkit #193

Implement tracing package for toolkit #193

Conversation

burov commented Jun 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewwells left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burov commented Jun 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mphillips-infoblox left a comment

Choose a reason for hiding this comment

Evgeniy-L left a comment

Choose a reason for hiding this comment

burov commented Jun 13, 2020 •

edited

Loading