close
The Wayback Machine - https://web.archive.org/web/20201217124155/https://github.com/mainflux/mainflux/issues/787
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

Enrich existing open tracing spans with additional data #787

Open
anovakovic01 opened this issue Jul 15, 2019 · 9 comments
Open

Enrich existing open tracing spans with additional data #787

anovakovic01 opened this issue Jul 15, 2019 · 9 comments
Assignees
Milestone

Comments

@anovakovic01
Copy link
Member

@anovakovic01 anovakovic01 commented Jul 15, 2019

FEATURE REQUEST

  1. Is there an open issue addressing this request? If it does, please add a "+1" reaction to the
    existing issue, otherwise proceed to step 2.
    No.

  2. Describe the feature you are requesting, as well as the possible use case(s) for it.
    After adding base open tracing support, spans should be enriched with additional tags. There is a recommendation of what should be added to the span and you can check it out here. One of the problems that we'll have to solve is how to write some kind of interceptor to the existing DB client in order to fetch SQL statements and passed params. The end goal should be removing separate logging and metrics middleware.

  3. Indicate the importance of this feature to you (must-have, should-have, nice-to-have).

@nwneisen
Copy link
Contributor

@nwneisen nwneisen commented Sep 16, 2019

I'd like to take a look at implementing this. I'm reading up on the Jaeger project and open traces and then should have a plan.

@nmarcetic
Copy link
Member

@nmarcetic nmarcetic commented Sep 16, 2019

Great @nwneisen :) Take your time for research, then we can sync here and agree about implementation.

@drasko
Copy link
Member

@drasko drasko commented Sep 16, 2019

@nwneisen fantastic!

What would be great also is that - once this is done - you document it through a blogpost (with nice screenshots), so that we have a documentation for other developers how to use OpenTarcing with Mainflux.

Your text will be then featured on Mainflux official blog: https://medium.com/mainflux-iot-platform (you can ping @Sale72 to help you with the draft process).

Also, it would be great to add a chapter in the official documentation as well.

@nwneisen
Copy link
Contributor

@nwneisen nwneisen commented Sep 18, 2019

After playing around with Jaeger and its examples, I think I've got my head wrapped around traces.

I found were spans are currently being created in mainflux. One such location is things/tracing/things.go. Here span.SetTag() can be used to add client information such as component name, span kind, and any errors. The transaction name (Save, Update, etc) also seems like useful information at this point but I don't see anything for it in the conventions document above.

From here the span will be passed to the database code at things/postgres/things.go via the context parameter. The span can be pulled out and have tags such as the query string and parameters added here. There are also some error cases that could be added to the span logs.

@anovakovic01
Copy link
Member Author

@anovakovic01 anovakovic01 commented Sep 18, 2019

@nwneisen yes, that is basically what we would like to have. The query string will be problematic to add to the span because driver creates end SQL statement that will be sent to the PostgreSQL under the hood. There are some interceptors (such as this one) that are being developed, but they are not stable IMHO and shouldn't be used in Mainflux. I'm not sure how we should approach this SQL statement logging problem.

@nwneisen
Copy link
Contributor

@nwneisen nwneisen commented Sep 20, 2019

I pushed a draft of my current changes. While I made sure everything built, make rundev seems to be a bit different than make run. I'll be taking a look at the differences so I can pull up Jaeger and trigger spans.

I put the code in place to pull the span from the context. Right now it it adds the SQL query before it has values in place. I can only think of two options for the sql.statement:

  1. Add the query string without values (what is in place now) along with tags for each individual value.
  2. Build the query string manually.

The first option would not follow the open tracing recommendations but would provide the same tracing information until an interceptor looks more stable. The second option doesn't really seem like an option as I think it suggests it is the final query to be ran on the database when it is not.

@nwneisen
Copy link
Contributor

@nwneisen nwneisen commented Oct 7, 2019

Finished this issue in #875 and #869. I just need to talk with @Sale72 about the draft process.

@chombium
Copy link
Member

@chombium chombium commented Nov 3, 2019

@nwneisen, @anovakovic01, @drasko as #869 has been merged, can this issue be closed or is there something more to do?

@nwneisen
Copy link
Contributor

@nwneisen nwneisen commented Nov 7, 2019

@chombium we weren't able to add all of the tags suggested in the specification. This might be still open to keep those additional tags in mind to be added later.

@drasko drasko added this to the 1.0.0 milestone Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.