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

Add stream interceptors #190

Merged
merged 7 commits into from
May 19, 2020
Merged

Add stream interceptors #190

merged 7 commits into from
May 19, 2020

Conversation

addudko
Copy link
Collaborator

@addudko addudko commented May 12, 2020

No description provided.

@Evgeniy-L Evgeniy-L requested review from burov and Evgeniy-L May 13, 2020 09:26
burov
burov approved these changes May 13, 2020
View reviewed changes
Copy link

@burov burov left a comment

Choose a reason for hiding this comment

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

Overall looks good, but can i ask to test this to be sure this works as expected, i don't have enought expirience with Streaming itnterception in gRPC

@Evgeniy-L Evgeniy-L requested a review from kd7lxl May 13, 2020 09:40
kd7lxl
kd7lxl reviewed May 13, 2020
View reviewed changes
Copy link
Contributor

@kd7lxl kd7lxl left a comment

Choose a reason for hiding this comment

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

This seems to duplicate a lot of functionality of the logrus StreamServerInterceptor with only minor changing in wording. Why not wrap and extend the existing middleware rather than replace it?

Please add UTs. This package is a significant regression from the coverage we had with the logrus middleware.

ok      github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus     0.335s  coverage: 79.1% of statements
ok      github.com/infobloxopen/atlas-app-toolkit/logging       0.299s  coverage: 11.9% of statements

burov
burov reviewed May 19, 2020
View reviewed changes
Copy link

@burov burov left a comment

Choose a reason for hiding this comment

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

Overall looks good, just a few minor comments

logging/interceptor.go
@@ -62,14 +62,14 @@ func LogLevelInterceptor(defaultLevel logrus.Level) grpc.UnaryServerInterceptor
}
}

func UnaryClientInterceptor(logger *logrus.Logger, opts ...Option) grpc.UnaryClientInterceptor {
func UnaryClientInterceptor(entry *logrus.Entry, opts ...Option) grpc.UnaryClientInterceptor {
options := initOptions(opts)

return func(ctx context.Context, method string, req, reply interface{}, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error {
startTime := time.Now()
fields := newLoggerFields(method, startTime, DefaultClientKindValue)
Copy link

Choose a reason for hiding this comment

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

I see we call newCtx := newLoggerForCall(stream.Context(), entry, fields) at another interceptors should we call it in this interceptor as well ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, if we're going to use client interceptor only for logging.
Also try to imagine situation: we populate fields in client interceptor, put logger with fields into context, make rpc call and on the server side we may only extract logger from context and not making same job again above those fields which already present. So I think we shouldn't impose folow that way to server.

logging/interceptor.go Outdated Show resolved Hide resolved
logging/interceptor.go Outdated Show resolved Hide resolved
logging/interceptor.go Outdated Show resolved Hide resolved
logging/interceptor.go Outdated Show resolved Hide resolved
logging/interceptor.go Outdated Show resolved Hide resolved
logging/interceptor.go Outdated Show resolved Hide resolved
logging/interceptor.go Outdated Show resolved Hide resolved
logging/interceptor.go Outdated Show resolved Hide resolved
Evgeniy-L
Evgeniy-L approved these changes May 19, 2020
View reviewed changes
Copy link
Contributor

@Evgeniy-L Evgeniy-L left a comment

Choose a reason for hiding this comment

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

overall looks good

logging/mocks/server_stream.go Show resolved Hide resolved
@Evgeniy-L Evgeniy-L merged commit c488b8e into infobloxopen:master May 19, 2020
This was referenced May 20, 2020
Open
Merged
abynenkov-ib added a commit that referenced this pull request Apr 27, 2021
@drewwells @daniel-garcia
@addudko @kd7lxl @abynenkov-ib @prybintsev @alavrovinfb @dependabot @binjip978
Update athena-identity version of atlas-app-toolkit. (#244)
8582628 
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@burov burov burov approved these changes

@Evgeniy-L Evgeniy-L Evgeniy-L approved these changes

@kd7lxl kd7lxl Awaiting requested review from kd7lxl

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@addudko @kd7lxl @Evgeniy-L @burov