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

*: types refactor #40

Merged
merged 1 commit into from
Jan 11, 2024
Merged

*: types refactor #40

merged 1 commit into from
Jan 11, 2024

Conversation

manasachi
Copy link
Contributor

@manasachi manasachi commented Jan 9, 2024

Modified struct definitions and removed test case for PercentileLatencies

@fuweid
Copy link
Collaborator

fuweid commented Jan 10, 2024

I think we need to do some refactors before introducing this feature.

1. Rethink ResponseStats.

We need to rethink about ResponseStats. I made a mistake on ResponseMetric.Gather().

Gather() (latencies []float64, percentileLatencies map[float64]float64, failureList []error, bytes int64)

This function should just return ResponseStats (raw data) instead of percentile latencies.
The caller should take responsibility to make summary on raw data as it wants.

So, I think ResponseStats should be like

// ResponseStats is the report about benchmark result.
type ResponseStats struct {
	// Total represents total number of requests.
	Total int
	// List of failures
	FailureList []error
        // All the observed latencies
	Latencies []float64
	// total bytes read from apiserver
	TotalReceivedBytes int64
}

// ResponseMetric is a measurement related to http response.
type ResponseMetric interface {
	...
	// Gather returns the summary.
	Gather() ResponseStats
}

I remove the Duration from ResponseStats because metric collector doesn't know when it starts and when it ends.
So, the Duration should be filled by Schedule.

func Schedule(ctx context.Context, spec *types.LoadProfileSpec, restCli []rest.Interface) (*types.ResponseStats, error) {

The Schedule should return their own type instead of ResponseStats.

// in request pkg

type Result struct {
       types.ResponseStats
       // Duration means the time of benchmark.
       Duration time.Duration
}

func Schedule(ctx context.Context, spec *types.LoadProfileSpec, restCli []rest.Interface) (*Result, error) {}

2. Introduce RunnerMetricReport type for command output

For command output, it's client facing. It could be changed or updated frequently.
So, we should introduce new type RunnerMetricReport for render purposes.
If there is no new raw metric, we don't need to touch ResponseStats.
All the aggregated features can be handled by updated RunnerMetricReport type.

// in api/types pkg

type RunnerMetricReport struct {
      // Total represents total number of requests.
	Total int  
	// List of failures
	FailureList []error
        // Duration means the time of benchmark.
        Duration time.Duration
        // All the observed latencies
	Latencies []float64
	// total bytes read from apiserver
	TotalReceivedBytes int64
        // PercentileLatencies represents the latency distribution in seconds.
        PercentileLatencies [][2]float64 // [2]float64{percentile, value}
}

3. Introduce --format=json and --raw-data flags together

Change current format into json. No need to maintain fmt.Fprintf() method.
If --raw-data is set, use json.NewEncoder() to render. If we don't have --raw-data, set Latencies field to nil and render it into json.

What do you think?

@manasachi manasachi changed the title Added --raw-data flag to export stats as .json *: types refactor Jan 10, 2024
modified response structs, removed percentileLatencies test cases
Copy link
Collaborator

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

@fuweid fuweid merged commit f0bf5ad into main Jan 11, 2024
4 checks passed
@fuweid fuweid deleted the mchinta/runner branch January 11, 2024 03:52
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.

[runner subcmd] add --raw-data flag to export raw data instead of aggregated one.
3 participants