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 a metrics package using OpenCensus, trace connections #2646

Merged
merged 1 commit into from
Jul 11, 2020

Conversation

marten-seemann
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Jul 6, 2020

Codecov Report

Merging #2646 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2646   +/-   ##
=======================================
  Coverage   86.68%   86.68%           
=======================================
  Files         123      123           
  Lines        9778     9778           
=======================================
  Hits         8476     8476           
  Misses        970      970           
  Partials      332      332           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f5cba0...f9eb10d. Read the comment docs.

)
}

func (t *connTracer) ClosedConnection(logging.CloseReason) {}

Choose a reason for hiding this comment

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

I know that opencensus doesn't technically have the concept of a gauge but I am pretty sure that you can just do a connection.M(-1). So we should be able to model activeConnections and not just the total number of connections that have been opened since the program started. There may be some issues with this approach if the metrics collector restarts and the process doesn't but I don't think so.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm a bit confused now. I had thought of this counter to be plotted as something like rate(connections[30s]), so it would measure the rate with which we're dialing and accepting new connections. Maybe connections is not the right name then?

If I understand correctly, what you're proposing is an active connection counter. I think I can introduce some counter variables into the tracer, then we don't have to worry about the metrics collector restarting.

Does that make sense to you?

Copy link
Member

@lucas-clemente lucas-clemente Jul 10, 2020

Choose a reason for hiding this comment

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

If you need a gauge, a better way of doing that could be keeping track of opened and closed connections (with the error code) separately. That allows you to implement a gauge as one of multiple possible downstream reports.

metrics/metrics.go Outdated Show resolved Hide resolved
)
}

func (t *connTracer) ClosedConnection(logging.CloseReason) {}
Copy link
Member

@lucas-clemente lucas-clemente Jul 10, 2020

Choose a reason for hiding this comment

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

If you need a gauge, a better way of doing that could be keeping track of opened and closed connections (with the error code) separately. That allows you to implement a gauge as one of multiple possible downstream reports.


// Tags
var (
keyPerspective, _ = tag.NewKey("perspective")
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about this, but another option here could be encoding server vs client in the counter name:

  • /quic/server/connections
  • /quic/server/var-that-only-makes-sense-for-servers
  • /quic/client/connections
  • /quic/client/var-that-only-makes-sense-for-clients

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's keep it as is for now. I'm just beginning to explore what we want to export, so this will probably change a couple of times anyway as we define more metrics.

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