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

Align configuration with OpenTelemetry SDK, HTTP exporter #5

Merged
merged 1 commit into from
Jun 3, 2024

Conversation

olegbespalov
Copy link
Collaborator

What?

This PR aligns configuration (k6 environment variables) with the OpenTelemetry SDK and introduces more configuration options, along with the HTTP metric exporter.

Why?

It's essential to be aligned so as not to confuse the users, as proven in the comment.

@olegbespalov olegbespalov self-assigned this May 23, 2024
@olegbespalov olegbespalov requested a review from a team as a code owner May 23, 2024 16:55
@olegbespalov olegbespalov requested review from mstoykov and joanlopez and removed request for a team May 23, 2024 16:55
@cwegener
Copy link

Looks good to me! 👍

Co-authored-by: Joan López de la Franca Beltran <5459617+joanlopez@users.noreply.github.com>
@olegbespalov olegbespalov changed the base branch from feat/rate to main May 24, 2024 11:10
Copy link
Contributor

@joanlopez joanlopez left a comment

Choose a reason for hiding this comment

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

I've added a few minor comments but overall looks great! 🚀

Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM!

I left two comments which can be addressed later as well - but might be good idea to happen before we put this in core.

Comment on lines 56 to +71
func NewConfig(p output.Params) (Config, error) {
cfg := Config{
MetricPrefix: "",
ReceiverType: grpcReceiverType,
GRPCReceiverEndpoint: "localhost:4317",
PushInterval: 1 * time.Second,
FlushInterval: 1 * time.Second,
ServiceName: "k6",
ServiceVersion: k6Const.Version,
MetricPrefix: "",
ExporterType: grpcExporterType,

HTTPExporterInsecure: null.BoolFrom(false),
HTTPExporterEndpoint: "localhost:4318",
HTTPExporterURLPath: "/v1/metrics",

GRPCExporterInsecure: null.BoolFrom(false),
GRPCExporterEndpoint: "localhost:4317",

ExportInterval: 1 * time.Second,
FlushInterval: 1 * time.Second,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: In general it is better idea to always use null types for everything as that lets you know if you should use a default value or if somebody just managed to match it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Implemented as part of: #11

Comment on lines 75 to +80
for k, v := range p.Environment {
switch k {
case "K6_OTEL_PUSH_INTERVAL":
cfg.PushInterval, err = time.ParseDuration(v)
if err != nil {
return cfg, fmt.Errorf("error parsing environment variable 'K6_OTEL_PUSH_INTERVAL': %w", err)
}
case "K6_OTEL_SERVICE_NAME":
cfg.ServiceName = v
case "K6_OTEL_SERVICE_VERSION":
cfg.ServiceVersion = v
Copy link
Contributor

Choose a reason for hiding this comment

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

Not certain if jut using envconfig.Process as all the other outputs will not be better especially before we move the whole configuration over to not using envconfig.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, implemented as part of: #11

@olegbespalov
Copy link
Collaborator Author

@mstoykov, both comments are relevant. I'll try to implement the following configuration-related PRs. This is mostly about aligning with OpenTelemetry terms, so will merge this as it is for now 👍

@olegbespalov olegbespalov merged commit 7083c18 into main Jun 3, 2024
10 checks passed
@olegbespalov olegbespalov deleted the feat/configuration branch June 3, 2024 12:04
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.

None yet

4 participants