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

[installer]: add jaeger sampling options to the tracing object #7727

Merged
merged 1 commit into from
Feb 2, 2022

Conversation

mrsimonemms
Copy link
Contributor

@mrsimonemms mrsimonemms commented Jan 20, 2022

Description

Adds Jaeger sampling options to the new experimental (and unsupported) config section

The config object is now:

observability:
  tracing: # The tracing object is required to activate the tracing option in the experimental
    endpoint: <url-of-endpoint> 
experimental:
  workspace:
    tracing:
      samplerType: const
      samplerParam: 1

This adds the ability to configure the sampler options for the tracing object. This validates the types/params against the Jaeger client

There is validation added to support this:

  • if tracing is omitted or samplerType or not configured, no error happens. This retains the same const and 1 default values as now
  • if tracing.samplerType is added, it ensures that the samplerType meets one of the options defined by Jaeger. It also requires the tracing.samplerParam, as this value is coupled to the samplerType

Related Issue(s)

Fixes #7698

How to test

Run validate config whilst changing the params to check the validation. Run render to generate a new Installer YAML

Release Notes

[installer]: add jaeger sampling options to the tracing object

Documentation

@mrsimonemms
Copy link
Contributor Author

/hold

@corneliusludmann I will need your approval for this to be merged. I would also like @aledbf's approval as well as this change has been requested by him

@codecov
Copy link

codecov bot commented Jan 20, 2022

Codecov Report

Merging #7727 (37d5cd2) into main (fb502be) will decrease coverage by 3.73%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             main   #7727      +/-   ##
=========================================
- Coverage   11.46%   7.73%   -3.74%     
=========================================
  Files          20      31      +11     
  Lines        1177    2199    +1022     
=========================================
+ Hits          135     170      +35     
- Misses       1039    2027     +988     
+ Partials        3       2       -1     
Flag Coverage Δ
components-gitpod-cli-app 10.20% <ø> (ø)
components-local-app-app-darwin-amd64 ?
components-local-app-app-darwin-arm64 ?
components-local-app-app-linux-amd64 ?
components-local-app-app-linux-arm64 ?
components-local-app-app-windows-386 ?
components-local-app-app-windows-amd64 ?
components-local-app-app-windows-arm64 ?
installer-raw-app 5.63% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
installer/pkg/common/common.go 4.45% <0.00%> (ø)
installer/pkg/components/ws-manager/deployment.go 0.00% <0.00%> (ø)
components/local-app/pkg/auth/auth.go
components/local-app/pkg/auth/pkce.go
installer/pkg/components/ws-manager/configmap.go 30.05% <0.00%> (ø)
...components/ws-manager/unpriviledged-rolebinding.go 0.00% <0.00%> (ø)
installer/pkg/common/display.go 0.00% <0.00%> (ø)
installer/pkg/components/ws-manager/rolebinding.go 0.00% <0.00%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb502be...37d5cd2. Read the comment docs.

@corneliusludmann corneliusludmann added the installer: needs interface change Change required to input, output or configuration file(s) label Jan 21, 2022
@corneliusludmann
Copy link
Contributor

cross-posting a comment I just added to the issue: #7698 (comment)

@mrsimonemms mrsimonemms requested a review from aledbf January 24, 2022 10:02
@mrsimonemms mrsimonemms force-pushed the sje/installer-observe-7698 branch from f282b1d to 8109412 Compare January 25, 2022 17:30
@mrsimonemms mrsimonemms requested review from corneliusludmann and a team January 26, 2022 11:24
Copy link
Contributor

@corneliusludmann corneliusludmann left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

Code looks code.

@roboquat roboquat added the lgtm label Jan 26, 2022
@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: 562cbb58a1bd4425024bd7417597dc689617b238

@mrsimonemms mrsimonemms force-pushed the sje/installer-observe-7698 branch from 8109412 to 21938f5 Compare January 27, 2022 15:24
@roboquat roboquat added team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team size/L and removed lgtm approved size/M labels Jan 27, 2022
@mrsimonemms mrsimonemms force-pushed the sje/installer-observe-7698 branch from 21938f5 to a56f2d8 Compare January 27, 2022 15:28
@mrsimonemms mrsimonemms force-pushed the sje/installer-observe-7698 branch from a56f2d8 to 37d5cd2 Compare January 27, 2022 16:34
@mrsimonemms
Copy link
Contributor Author

/unhold

Copy link
Contributor

@sagor999 sagor999 left a comment

Choose a reason for hiding this comment

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

@aledbf is on vacation, so approving on his behalf. LGTM.

@roboquat
Copy link
Contributor

roboquat commented Feb 1, 2022

LGTM label has been added.

Git tree hash: 80e1d8e96bce3a7cb2e6d171476f33ebdaf2d674

@mrsimonemms
Copy link
Contributor Author

/unhold

@mrsimonemms mrsimonemms requested a review from a team February 1, 2022 19:47
Copy link
Contributor

@JanKoehnlein JanKoehnlein left a comment

Choose a reason for hiding this comment

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

LGTM

@roboquat
Copy link
Contributor

roboquat commented Feb 2, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: corneliusludmann, JanKoehnlein, sagor999

Associated issue: #7698

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 1b03d7a into main Feb 2, 2022
@roboquat roboquat deleted the sje/installer-observe-7698 branch February 2, 2022 10:21
@roboquat roboquat added the deployed: workspace Workspace team change is running in production label Feb 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved deployed: workspace Workspace team change is running in production installer: needs interface change Change required to input, output or configuration file(s) release-note size/L team: delivery Issue belongs to the self-hosted team team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[installer] There is no option to set a sampling strategy in the observability section
7 participants