-
Notifications
You must be signed in to change notification settings - Fork 7
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.
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
Remove sapm handler and refactor for remaining sapm parser #7
Conversation
} | ||
|
||
// NewTraceHandlerV2 returns an http.HandlerFunc for receiving SAPM requests and passing the SAPM to a receiving function | ||
func NewTraceHandlerV2(receiver func(ctx context.Context, sapm *splunksapm.PostSpansRequest, err error) error) func(rw http.ResponseWriter, req *http.Request) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this func no longer needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No no one is using it directly. The problem was that there's no good way to pass a context into a downstream http handler func and still conform to the handlerfunc interface. Basically to do it properly we would have to write our own middleware interface for linking handlers together and passing things like context. In the end @owais and I decided it would be better to just add the functionality directly into the handler in the otel receiver. This was ok, because @mdubbyap wasn't using this handler anyway. He is just using the parse function.
sapmrequestparser/parser.go
Outdated
|
||
const ( | ||
// TraceEndpointV2 is the endpoint used for SAPM v2 traces. The SAPM protocol started with v2. There is no v1. | ||
TraceEndpointV2 = "/v2/trace" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we using this and other vars anymore? If not, may be move them to tests instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would vote to keep them and even make public/exported so that whoever implements SAPM client or server can use these pre-defined consts. These are part of protocol specification. Maybe only name them slightly differently, e.g.
ContentTypeHeaderName = "Content-Type"
ContentTypeHeaderValue = "application/x-protobuf"
etc...
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way SAPM implementations will refer to them as sapmrequestparser.ContentTypeHeaderName
, probably not ideal but should be fine (something like sapmprotocol.ContentTypeHeaderName
would look nicer, maybe put them in a different package?). Anyway up to you @codesmith14 , these are nits.
I missed the comment about renaming the package to sapmprotocol. I'll do that real quick. |
Ok, refactored the package name. @tigrannajaryan @owais I'll merge and then update the contrib pr pending your approval. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.