Fixes and refactors trace v2 #288
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The trace v2 package was not working properly. It didn't support naming the system and the integration with another packages were broken, e.g., not being injected on the logs. I tried to solve all the issues without breaking too much compatibility but some breaking changes on trace v2 were made. The tracking middleware were coupling request and trace packages. I split the middleware into two (the old one still exists but now is deprecated), so request and trace has each one its own middleware. The request middleware should be used after the trace middleware so it can inject the request-id into the trace. The previous middleware supports both versions of trace so it can be used during the phasing out of v1. The span object in trace v2 was dropped in favor of the otel span directly. The otel span is already an interface and it is very powerful. If we would wrap this interface we would need to write too much code and still would lose some of its power. Because otel is a good market standard I felt confident in exposing it's span and use the trace v2 package only as an orquestrator. The example was also redone and it's now complete dockerized and using jaeger to collect and serve the traces. A new gokit endpoint middleware was created that starts a trace for the endpoint but also handle errors and allows for custom attributes to be inserted on the span.
leoeareis
approved these changes
Nov 23, 2023
eric-reis
reviewed
Nov 24, 2023
request/http.go
Outdated
| // HTTPMiddleware returns an http middleware that adds a request id | ||
| // to the context of the request and the same id in the header of the | ||
| // http response. If there is an active trace span, the request id is | ||
| // also registred as an attribute 'request.id'. It's importante that |
There was a problem hiding this comment.
Nit: "importante" -> "important"
eric-reis
reviewed
Nov 24, 2023
trace/v2/config.go
Outdated
| } | ||
|
|
||
| if err != nil { | ||
| log.Fatal().Str("exporter", exporterName).Err(err).Msg("Failed to create trace exporter") | ||
| return nil, fmt.Errorf("creating trace exporter: %w", err) |
There was a problem hiding this comment.
Nit: here it could use errors.Errorf in order to avoid importing fmt, as errors is already imported
eric-reis
reviewed
Nov 24, 2023
trace/v2/config.go
Outdated
| func newResource(serviceName, serviceVersion string) (*resource.Resource, error) { | ||
| return resource.Merge(resource.Default(), | ||
| resource.NewWithAttributes(semconv.SchemaURL, | ||
| semconv.ServiceName(serviceName), |
There was a problem hiding this comment.
Are serviceName and serviceVersion required? If they are, does resource return a good error message? If not, a custom error message would make it quick to someone who is using fkit without deep knowledge of this source code to know which configurations are missing
eric-reis
approved these changes
Nov 24, 2023
The Dockerfile was dropped and the docker-compose was enhanced due to security issues raised by sonar. Custom configuration was dropped in favor of the official environment variables. README as converted into a proper godoc. Added some sanity checks for checking for missing environment variables when the default OpenTelemetry values are not ideal.
eric-reis
reviewed
Nov 29, 2023
| OTEL_TRACES_SAMPLER: "parentbased_traceidratio" | ||
| OTEL_TRACES_SAMPLER_ARG: "0.5" | ||
|
|
||
| # TraceV1 is only used for compatibility demontration |
There was a problem hiding this comment.
Nit: demontration -> demonstration
eric-reis
approved these changes
Nov 29, 2023
The cache was supposed to be committed commented out.
|
Kudos, SonarCloud Quality Gate passed!
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The trace v2 package was not working properly. It didn't support naming the system and the integration with another packages were broken, e.g., not being injected on the logs.
I tried to solve all the issues without breaking too much compatibility but some breaking changes on trace v2 were made.
The tracking middleware were coupling request and trace packages. I split the middleware into two (the old one still exists but now is deprecated), so request and trace has each one its own middleware. The request middleware should be used after the trace middleware so it can inject the request-id into the trace.
The previous middleware supports both versions of trace so it can be used during the phasing out of v1.
The span object in trace v2 was dropped in favor of the otel span directly. The otel span is already an interface and it is very powerful. If we would wrap this interface we would need to write too much code and still would lose some of its power. Because otel is a good market standard I felt confident in exposing it's span and use the trace v2 package only as an orquestrator.
The example was also redone and it's now complete dockerized and using jaeger to collect and serve the traces.
A new gokit endpoint middleware was created that starts a trace for the endpoint but also handle errors and allows for custom attributes to be inserted on the span.