Skip to content

Refactored instrumented; Added profiling; better error handling#49

Merged
bwplotka merged 7 commits intomainfrom
mon
Sep 14, 2022
Merged

Refactored instrumented; Added profiling; better error handling#49
bwplotka merged 7 commits intomainfrom
mon

Conversation

@bwplotka
Copy link
Contributor

NOTE: This breaks API, but it should be now more consistent, easier to use and easier to extend.

I tried different approaches, but I found that splitting extensions to different packages (e.g profiling and monitoring) and simplifying will work the best. Let me know what you think!

Essentially this replaces definition like this:

return e2e.NewInstrumentedRunnable(env, name).WithPorts(map[string]int{AccessPortName: 2379, "metrics": 9000}, "metrics").Init(
		e2e.StartOptions{
			Image:     o.image,
			Command:   e2e.NewCommand("/usr/local/bin/etcd", "--listen-client-urls=http://0.0.0.0:2379", "--advertise-client-urls=http://0.0.0.0:2379", "--listen-metrics-urls=http://0.0.0.0:9000", "--log-level=error"),
			Image: o.image,
			Command: e2e.NewCommand(
				"/usr/local/bin/etcd",
				"--listen-client-urls=http://0.0.0.0:2379",
				"--advertise-client-urls=http://0.0.0.0:2379",
				"--listen-metrics-urls=http://0.0.0.0:9000",
				"--log-level=error",
			),
			Readiness: e2e.NewHTTPReadinessProbe("metrics", "/health", 200, 204),
		},
)

with

return e2emon.AsInstrumented(env.Runnable(name).WithPorts(map[string]int{AccessPortName: 2379, "metrics": 9000}).Init(
		e2e.StartOptions{
			Image:     o.image,
			Command:   e2e.NewCommand("/usr/local/bin/etcd", "--listen-client-urls=http://0.0.0.0:2379", "--advertise-client-urls=http://0.0.0.0:2379", "--listen-metrics-urls=http://0.0.0.0:9000", "--log-level=error"),
			Image: o.image,
			Command: e2e.NewCommand(
				"/usr/local/bin/etcd",
				"--listen-client-urls=http://0.0.0.0:2379",
				"--advertise-client-urls=http://0.0.0.0:2379",
				"--listen-metrics-urls=http://0.0.0.0:9000",
				"--log-level=error",
			),
			Readiness: e2e.NewHTTPReadinessProbe("metrics", "/health", 200, 204),
		},
	)
), "metrics")

Not a big deal from usage, but much more cleaner when we start adding other extension to runnables like profiling.

Signed-off-by: bwplotka <bwplotka@gmail.com>
Signed-off-by: bwplotka <bwplotka@gmail.com>
Signed-off-by: bwplotka <bwplotka@gmail.com>
from


return e2emonitoring.NewInstrumentedRunnable(env, name).WithPorts(ports, "http").Init(e2e.StartOptions{
		Image:     o.image,
		Command:   e2e.NewCommand("sidecar", e2e.BuildKingpinArgs(args)...),
		Readiness: e2e.NewHTTPReadinessProbe("http", "/-/ready", 200, 200),
		User:      strconv.Itoa(os.Getuid()),
	})

to

return e2emonitoring.AsInstrumented(env.Runnable(name).WithPorts(ports).Init(e2e.StartOptions{
		Image:     o.image,
		Command:   e2e.NewCommand("sidecar", e2e.BuildKingpinArgs(args)...),
		Readiness: e2e.NewHTTPReadinessProbe("http", "/-/ready", 200, 200),
		User:      strconv.Itoa(os.Getuid()),
	}), "http")


Signed-off-by: bwplotka <bwplotka@gmail.com>
Signed-off-by: bwplotka <bwplotka@gmail.com>
Copy link
Collaborator

@saswatamcode saswatamcode left a comment

Choose a reason for hiding this comment

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

Thanks! This looks awesome! 🌟

One minor comment.

Copy link
Collaborator

@jessicalins jessicalins left a comment

Choose a reason for hiding this comment

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

cool refactor, I think it makes sense to split in different packages according with different responsibilities, as you did here :)
regarding breaking the current API - is being backwards compatible something we would pay attention to? or once this is merged and a new tag is created, those changes will be communicated in the release?
I left some suggestions

@bwplotka
Copy link
Contributor Author

regarding breaking the current API - is being backwards compatible something we would pay attention to? or once this is merged and a new tag is created, those changes will be communicated in the release?

Correct, plus we are not v1.x, yet so we can change things.

bwplotka and others added 2 commits September 14, 2022 16:17
Co-authored-by: Jéssica Lins  <jessicaalins@gmail.com>
Signed-off-by: bwplotka <bwplotka@gmail.com>
@bwplotka bwplotka merged commit 0572685 into main Sep 14, 2022
@bwplotka bwplotka deleted the mon branch September 14, 2022 16:49
@@ -264,11 +256,3 @@ func (r *instrumentedRunnable) WaitRemovedMetric(metricName string, opts ...Metr

return errors.Newf("the metric %s is still exported by %s", metricName, r.Name())
}

func NewErrInstrumentedRunnable(name string, err error) InstrumentedRunnable {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the replacement for this now? Trying to upgrade the version on the Thanos side

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's InstrumentedRunnable{Runnable: e2e.NewError... (:

}

func (s *Service) GetMonitoringRunnable() e2e.InstrumentedRunnable {
return s.p.InstrumentedRunnable
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removing this breaks the meta monitoring E2E test so probably we need to add it back.

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.

4 participants