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.

Sign up for GitHub

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

Jump to bottom

Made tracing optional for preview environments #7020

Merged
merged 1 commit into from
Dec 6, 2021
Merged

Made tracing optional for preview environments #7020

merged 1 commit into from
Dec 6, 2021

Conversation

fullmetalrooster
Copy link
Contributor

@fullmetalrooster fullmetalrooster commented Dec 2, 2021 •
edited
Loading

Description

This makes tracing optional for preview-environments and binds its deployment to the observability stack.

Related Issue(s)

Fixes https://github.com/gitpod-io/ops/issues/334

How to test

Start a prev-environment without and with-observability.

Release Notes

NONE

@roboquat roboquat added the size/S label Dec 2, 2021
@fullmetalrooster fullmetalrooster force-pushed the wth/tracing-optional branch 2 times, most recently from f75b167 to f597616 Compare December 2, 2021 09:36
mads-hartmann
mads-hartmann requested changes Dec 2, 2021
View reviewed changes
Copy link
Contributor

@mads-hartmann mads-hartmann left a comment

Choose a reason for hiding this comment

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

I believe this is a breaking change to the Helm chart. Please see comment ☺️

chart/templates/_helpers.tpl Outdated
@@ -258,7 +258,7 @@ env:
{{- $gp := .gp -}}
{{- $comp := .comp -}}
{{- $tracing := $comp.tracing | default $gp.tracing -}}
{{- if $tracing }}
Copy link
Contributor

@mads-hartmann mads-hartmann Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think this might be a breaking change. I believe that all users of the Helm chart that previous has tracing enabled by setting tracing.endpoint would now have to update their values files to also have tracing.enabled

If so I would rather not go down this route and instead go with the suggested solution in https://github.com/gitpod-io/ops/issues/334 and just use additional values files for preview environments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I change it, but it is common to use .enabled to switch optional deployments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@roboquat roboquat added size/XS and removed size/S labels Dec 2, 2021
@ArthurSens
Copy link
Contributor

Wulf, our changelog automation relies on us setting the release notes to the PR description. Could you add it, please?

just add this to your PRs (Edit this comment and copy the content below):

NONE

@ArthurSens
Copy link
Contributor

ArthurSens commented Dec 2, 2021 •
edited by werft-gitpod-dev-com bot
Loading

Also, I think you meant

Fixes https://github.com/gitpod-io/ops/issues/334 instead of Fixes #334 🙂

@fullmetalrooster fullmetalrooster force-pushed the wth/tracing-optional branch 2 times, most recently from bdca791 to 0781385 Compare December 2, 2021 13:07
mads-hartmann
mads-hartmann approved these changes Dec 2, 2021
View reviewed changes
Copy link
Contributor

@mads-hartmann mads-hartmann left a comment

Choose a reason for hiding this comment

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

The code change looks good to me. I added the hold label so that Prow won't merge. Please verify that with your latest code change the error logs from e.g. ws-manger-bridge are not longer there as per the "How to reproduce" section of the issue

@roboquat roboquat added the lgtm label Dec 2, 2021
@roboquat
Copy link
Contributor

roboquat commented Dec 2, 2021

LGTM label has been added.

Git tree hash: 0074564f274ce068c54c31c1735ef92c45dd507c

@roboquat roboquat removed the lgtm label Dec 3, 2021
ArthurSens
ArthurSens approved these changes Dec 3, 2021
View reviewed changes
Copy link
Contributor

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

LGTM for me as well, now let's hunt for an OWNER's approval 😅

@roboquat roboquat added the lgtm label Dec 3, 2021
@roboquat
Copy link
Contributor

roboquat commented Dec 3, 2021

LGTM label has been added.

Git tree hash: 0074564f274ce068c54c31c1735ef92c45dd507c

@mads-hartmann
Copy link
Contributor

/assign @princerachit

Per quest by roboquat

@princerachit
Copy link
Contributor

princerachit commented Dec 6, 2021 •
edited
Loading

Hi I could not fully understand this part of the description:
...binds its deployment to the observability stack....
I can see that we are using the flag to control configuration of tracing for helm installation but don't understand how does it make tracing optional. Do you mean that monitoring satellite will continue to deploy tracing components like otel but due to this flag the actual traces will never travel to honeycomb?

@fullmetalrooster
Copy link
Contributor Author

If you add "with-observabiliy" you get tracing, if you do not then there will be no tracing at all.

@princerachit
Copy link
Contributor

princerachit commented Dec 6, 2021 •
edited
Loading

If you add "with-observabiliy" you get tracing, if you do not then there will be no tracing at all.

Ok, so in abstraction I get it. But monitoring satellite will continue deploying the tracing components, right?

Just want to be sure I understand the technicality of the changes

@princerachit
Copy link
Contributor

/approve

@roboquat
Copy link
Contributor

roboquat commented Dec 6, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ArthurSens, mads-hartmann, princerachit

Associated issue: #334

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roboquat roboquat merged commit c0b7a94 into main Dec 6, 2021
@roboquat roboquat deleted the wth/tracing-optional branch December 6, 2021 08:48
@mads-hartmann
Copy link
Contributor

mads-hartmann commented Dec 6, 2021 •
edited by werft-gitpod-dev-com bot
Loading

@princerachit You only get monitoring-satellite if you use with-observability. This PR fixes the Gitpod configuration so that tracing is only configured if we have somewhere to send them ☺️ The issue gitpod-io/ops#334 has a bit more context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ArthurSens ArthurSens ArthurSens approved these changes

@mads-hartmann mads-hartmann mads-hartmann approved these changes

Assignees

@mads-hartmann mads-hartmann

@ArthurSens ArthurSens

@princerachit princerachit

Labels
approved release-note size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fullmetalrooster @ArthurSens @roboquat @mads-hartmann @princerachit