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

add meaningful http tags in tracing handler #237

Conversation

bvillanueva-mdsol
Copy link
Contributor

@bvillanueva-mdsol bvillanueva-mdsol commented Apr 1, 2020

add meaningful tag information in tracing handler to be logged in client trace span

Example client span:

{
    "traceId": "32fbe6aebee305d6",
    "parentId": "9226c0ff90576f24",
    "id": "45ac2aef4e520137",
    "kind": "CLIENT",
    "name": "post",
    "timestamp": 1585705152801537,
    "duration": 2271648,
    "localEndpoint": {
        "serviceName": "xxx.com",
        "ipv4": "10.152.25.184"
    },
    "tags": {
        "http.host": "yyy.com",
        "http.method": "POST",
        "http.path": "/index",
        "http.status_code": "200"
    }
}

Please review and merge.
@adriancole @jcarres-mdsol

FYI @kfurue-mdsol @ykitamura-mdsol @sleeloy-mdsol @lschreck-mdsol

@jcarres-mdsol
Copy link
Contributor

@adriancole does it sound right?

@codefromthecrypt
Copy link
Member

by default we usually don't add host. also only non-success status codes. otherwise sounds good!

@bvillanueva-mdsol
Copy link
Contributor Author

bvillanueva-mdsol commented Apr 3, 2020

Added code to only log non-success status codes and unit tests too 66c64a1
@adriancole @jcarres-mdsol

@codefromthecrypt
Copy link
Member

codefromthecrypt commented Apr 3, 2020 via email

@bvillanueva-mdsol
Copy link
Contributor Author

Thanks for the advice. Add option to log http host with unit test
2605829
@adriancole @jcarres-mdsol

@jcarres-mdsol jcarres-mdsol merged commit 5ee6e43 into openzipkin:master Apr 4, 2020
@bvillanueva-mdsol bvillanueva-mdsol deleted the feature/add-http-tags-in-tracing-handler branch April 5, 2020 12:26
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.

3 participants