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.

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

Merged
merged 3 commits into from
Dec 6, 2019

Conversation

codesmith14
Copy link
Contributor

No description provided.

}

// 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) {
Copy link
Collaborator

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?

Copy link
Contributor Author

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.


const (
// TraceEndpointV2 is the endpoint used for SAPM v2 traces. The SAPM protocol started with v2. There is no v1.
TraceEndpointV2 = "/v2/trace"
Copy link
Contributor

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?

Copy link
Collaborator

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM

Copy link
Collaborator

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.

@codesmith14
Copy link
Contributor Author

I missed the comment about renaming the package to sapmprotocol. I'll do that real quick.

@codesmith14
Copy link
Contributor Author

codesmith14 commented Dec 6, 2019

Ok, refactored the package name. @tigrannajaryan @owais I'll merge and then update the contrib pr pending your approval.

Copy link
Collaborator

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM

@codesmith14 codesmith14 merged commit c977e7d into master Dec 6, 2019
@bogdandrutu bogdandrutu deleted the parse branch June 11, 2020 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants