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

Times should be stored as time.Time #24

Open
earlgreyz opened this issue Jul 24, 2018 · 6 comments
Open

Times should be stored as time.Time #24

earlgreyz opened this issue Jul 24, 2018 · 6 comments
Assignees

Comments

@earlgreyz
Copy link
Contributor

Duration should be measured using monotonic clocks, this can be done by using standard time.Time and time.Duration instead of storing int64 unix epoch times. https://golang.org/pkg/time/#hdr-Monotonic_Clocks

@earlgreyz
Copy link
Contributor Author

I can prepare a PR if it is agreed on, but it will most likely break the backwards compatibility.

@jparise
Copy link
Collaborator

jparise commented Jul 24, 2018

I think that would be a good improvement. Any opinion, @cgordon?

@cgordon
Copy link
Collaborator

cgordon commented Jul 24, 2018

That is a great idea. I've been working on a new version of bender that uses more idiomatic Go (and less memory). I have rewritten the core loop (and have been using it for a new project), but I haven't (yet) had time to rewrite the HTTP and Thrift tutorials to use it. You can take a look here: https://github.com/cgordon/bender.

As I get time this week, I'll try to add some other new things, including a simple JSON output format for the timing events and some Python tools to generate useful statistics from that output.

@earlgreyz
Copy link
Contributor Author

@cgordon since you've been working on a new version could you consider allowing user to provide their own time measure checkpoints (optional)?

Setup before sending a request might take a long time and interfere the results. I've encountered that while doing simple UDP loadtests (I've stripped not important parts):

func UDPExecutor(_ int64, request interface{}) (interface{}, error) {
	datagram := request.(*Datagram)

	// This part can take quite long and we'd like to ignore it in time measures
	conn, err := net.Dial("udp", addr)
	if err != nil {
		return nil, err
	}
	defer conn.Close()

	// Start measure here
	conn.SetWriteDeadline(time.Now().Add(timeout))
	_, err = conn.Write(datagram.Data)
	if err != nil {
		return nil, err
	}

	buffer := make([]byte, bufferSize)
	conn.SetReadDeadline(time.Now().Add(timeout))
	n, err := conn.Read(buffer)
	if err != nil {
		return nil, err
	}

	return buffer[:n], nil
}

My "temporary fix" was to calculate it manually and return the results as a struct

// Response represents a udp response received
type Response struct {
	Start   time.Time
	End     time.Time
	Elapsed time.Duration
	Data    []byte
}

But it also means that histogram and other recorders need to be adjusted for this new format. It would be really good to have a generic solution to this.

@cgordon
Copy link
Collaborator

cgordon commented Jul 26, 2018

@earlgreyz That's an interesting idea. I've been having the request generator do all the initialization for our load testing, and passing everything into the executor, but I can see how you may want to avoid that here. It looks like the easiest option to make this work would be to redefine the RequestExecutor to return four values: the response, the start time, the end time and an error. Then, rather than the main loop tracking the start and end time, the request executor would need to do so. In that case, it would also be easy to write a little wrapper function that did the timing (for people who don't need that fine control). Does that sound like it would do what you want?

@earlgreyz
Copy link
Contributor Author

@cgordon yes, that seems like a good solution

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

No branches or pull requests

3 participants