[builder] Remove builder default confmap providers #11126

bogdandrutu · 2024-09-10T21:43:10Z

Description

The main problem with this, is that the providers will start to have a different version than the builder and they will be independently mark as stable which will make it very hard to track all combination of builder versions with the "associated default" providers for that version.

This also fixes an issue, that for builder 0.109.0 the default providers will not work since the env and file are no longer using 0.109.0 version.

codecov · 2024-09-10T21:59:29Z

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.91%. Comparing base (b06236c) to head (9090546).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/builder/internal/builder/config.go	50.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11126      +/-   ##
==========================================
- Coverage   91.92%   91.91%   -0.01%     
==========================================
  Files         430      430              
  Lines       20311    20292      -19     
==========================================
- Hits        18671    18652      -19     
  Misses       1267     1267              
  Partials      373      373

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

atoulme · 2024-09-10T22:00:48Z

cmd/builder/internal/builder/config.go

-			return err
-		}
-		c.Providers = &providers
-	} else {


is it a problem if no providers are defined, should the builder error out in that case?

TylerHelmuth · 2024-09-10T22:01:53Z

cmd/builder/internal/builder/config.go

-			return err
-		}
-		c.Providers = &providers
+	c.Providers, err = parseModules(c.Providers)


I fear this will break most ocb users. If we go this route, is there a gentler approach we could take with feature gates or timelines?

The main problem with this, is that the providers will start to have a different version than the builder and they will be independently mark as stable which will make it very hard to track all combination of builder versions with the "associated default" providers for that version.

This also fixes an issue, that for builder 0.109.0 the default providers will not work since the env and file are no longer using 0.109.0 version.

I would have suggested to actually wait for all providers to be marked stabled in one version, that would have simplified a lot. Otherwise, I am not sure.

I fear this will break most ocb users. If we go this route, is there a gentler approach we could take with feature gates or timelines?

Also, right now they are broke anyway. Because if they don't specify any provider, it will try to use env/file at 109 and will fail.

I don't think there's much that prevents us from marking the other providers as 1.0, I will open the PR for those

I am 100% to add support in the future for featureflags.

For this particular change, I am fine with any of

an error message that tells you what do very explicitly

a feature gate

a dedicated config flag

The "at least one provider is required" may not be super clear to people (they may not be aware of what providers they used to put in there).

an error message that tells you what do very explicitly

Already there, tell me how you want the error to be, happy to change to a more clear. I think you only care about the message correct?

Yeah, just something that tells you "see example on https://opentelemetry.io/docs/collector/custom-collector/" or "previous default was [snippet of code that you can almost copy-paste]" would work for me

Agreed. Since this breaking change will affect end users of OCB who were not already setting providers, they'll be the most confused about what a provider is and what they need to set. We need to make it obvious what to do from the error message and the changelog entry (and I believe the OCB readme as well), so that they can learn how to fix their manifest.

mx-psi

I don't think we should do this in one step, and definitely not without updating all our examples and our own manifests in -releases.

bogdandrutu · 2024-09-11T12:38:54Z

@mx-psi please propose a solution when blocking. We have a bug right now and we need a solution, this is my proposal to that, do you have an alternative?

mx-psi · 2024-09-11T13:04:29Z

@mx-psi please propose a solution when blocking. We have a bug right now and we need a solution, this is my proposal to that, do you have an alternative?

For the immediate term, I can think of three options:

Publishing a v0.109.0 for these two providers with the same contents as v1.15.0
Hardcoding the versions of these two providers on the builder
Fetching the versions.yaml file from Github and checking what's the v1.x version associated with a particular v0.x

I think doing (1) is harmless, fixes the issue for everybody and gives us more time

bogdandrutu · 2024-09-11T13:46:51Z

I like 1 the most. I don't think 3 is feasible, I don't really like 2 because will grow a lot.

Publishing a v0.109.0 for these two providers with the same contents as v1.15.0

codeboten · 2024-09-11T14:09:35Z

Publishing a v0.109.0 for these two providers with the same contents as v1.15.0

I agree that seems like the solution that will impact end users the least, i don't know if there are any side effects of publishing the same package under two different tags... i can't think of one, but unexpected go dependencies issues have crept up in the past

codeboten · 2024-09-11T14:13:34Z

Linking user report of this bug here #11129

mx-psi · 2024-09-11T14:49:08Z

Publishing a v0.109.0 for these two providers with the same contents as v1.15.0

I agree that seems like the solution that will impact end users the least, i don't know if there are any side effects of publishing the same package under two different tags... i can't think of one, but unexpected go dependencies issues have crept up in the past

@codeboten I created https://github.com/mx-psi/go-double-tagging to allow tests with this. v0.2.0 and v1.0.0 point to the same commit.

With a program like this (main.go):

package main

import (
  "fmt"
  "github.com/mx-psi/go-double-tagging"
)

func main() {
	fmt.Println(godoubletagging.SomeSymbol)
}

and a go.mod like this with any version (I tagged v0.1.0, v0.2.0 which is also v1.0.0, v1.1.0):

module example.com

go 1.22.4

require github.com/mx-psi/go-double-tagging <put any valid tag here>

I can go mod tidy, go run main.go and I get the expected output.

Not a definitive proof but everything seems to be working fine.

github-actions · 2024-10-03T03:17:14Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

mx-psi · 2024-10-03T10:12:14Z

After #11193 and #11194 all provider modules we include in core are now stable, so we can revisit this.

To be honest, this has been broken for a few releases and we haven't gotten a lot of complaints, so maybe it's fine to just go with this change if we give clear migration instructions. I am going to remove my 'request changes'.

The bug hasn't been a big complaint from users, so it may be fine to just go with this

bogdandrutu · 2024-10-09T00:16:24Z

@mx-psi @codeboten @dmitryax I would propose to move forward with this and remove the problems in the future.

dmitryax · 2024-10-09T00:53:19Z

I agree. It seems better to be explicit here. Providers should not be something strange to the OCB users. This PR LGTM

mx-psi · 2024-10-09T13:44:15Z

.chloggen/fixbuilderproviders.yaml

+component: 'builder'
+
+# A brief description of the change.  Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Remove default config providers. This will require users to always specify the providers.


Can we add subtext with what users have to specify to keep the previous behavior?

Agreed. This is going to break a lot of OCB users, I'd really like to make it as easy as possible to fix which means supplying as much help as possible.

Agreed. This is going to break a lot of OCB users, I'd really like to make it as easy as possible to fix which means supplying as much help as possible.

I don't believe it, recent data show that is not as big of a deal.

mx-psi · 2024-10-09T13:44:49Z

Let's give @TylerHelmuth one week to reply since he raised concerns about this

TylerHelmuth · 2024-10-09T19:37:31Z

To be honest, this has been broken for a few releases and we haven't gotten a lot of complaints, so maybe it's fine to just go with this change if we give clear migration instructions. I am going to remove my 'request changes'.

@mx-psi no one else complained bc we released both stable and beta tags right? So the current default providers worked.

bogdandrutu · 2024-10-09T19:43:28Z

@mx-psi no one else complained bc we released both stable and beta tags right? So the current default providers worked.

They were broken for a long time.

mx-psi · 2024-10-09T19:46:54Z

To be honest, this has been broken for a few releases and we haven't gotten a lot of complaints, so maybe it's fine to just go with this change if we give clear migration instructions. I am going to remove my 'request changes'.

@mx-psi no one else complained bc we released both stable and beta tags right? So the current default providers worked.

My understanding is that we did not fix it for v0.110.0, e.g. see https://github.com/open-telemetry/opentelemetry-collector/releases/tag/confmap%2Fprovider%2Fenvprovider%2Fv0.110.0 (not found).

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

bogdandrutu requested review from a team and mx-psi September 10, 2024 21:43

bogdandrutu force-pushed the fixbuilderproviders branch 2 times, most recently from eb2aa45 to 4dec027 Compare September 10, 2024 21:51

bogdandrutu added the bug Something isn't working label Sep 10, 2024

atoulme reviewed Sep 10, 2024

View reviewed changes

TylerHelmuth reviewed Sep 10, 2024

View reviewed changes

bogdandrutu changed the title ~~[builder] Remove builder defaulr confmap providers~~ [builder] Remove builder default confmap providers Sep 11, 2024

mx-psi previously requested changes Sep 11, 2024

View reviewed changes

github-actions bot added the Stale label Oct 3, 2024

github-actions bot removed the Stale label Oct 4, 2024

bogdandrutu force-pushed the fixbuilderproviders branch from 4dec027 to 9090546 Compare October 9, 2024 00:15

bogdandrutu requested a review from a team as a code owner October 9, 2024 00:15

bogdandrutu requested a review from songy23 October 9, 2024 00:15

dmitryax approved these changes Oct 9, 2024

View reviewed changes

mx-psi approved these changes Oct 9, 2024

View reviewed changes

mx-psi requested a review from TylerHelmuth October 9, 2024 13:44

[builder] Remove builder defaulr confmap providers

98b2dc1

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

bogdandrutu force-pushed the fixbuilderproviders branch from 9090546 to 98b2dc1 Compare October 9, 2024 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[builder] Remove builder default confmap providers #11126

[builder] Remove builder default confmap providers #11126

bogdandrutu commented Sep 10, 2024 •

edited

Loading

codecov bot commented Sep 10, 2024 •

edited

Loading

atoulme Sep 10, 2024

TylerHelmuth Sep 10, 2024

bogdandrutu Sep 10, 2024

bogdandrutu Sep 10, 2024

bogdandrutu Sep 10, 2024

mx-psi Sep 11, 2024

bogdandrutu Oct 12, 2024

mx-psi Oct 14, 2024

bogdandrutu Oct 14, 2024

mx-psi Oct 14, 2024

TylerHelmuth Oct 15, 2024

mx-psi left a comment

bogdandrutu commented Sep 11, 2024

mx-psi commented Sep 11, 2024

bogdandrutu commented Sep 11, 2024

codeboten commented Sep 11, 2024

codeboten commented Sep 11, 2024 •

edited

Loading

mx-psi commented Sep 11, 2024 •

edited

Loading

github-actions bot commented Oct 3, 2024

mx-psi commented Oct 3, 2024

bogdandrutu commented Oct 9, 2024

dmitryax commented Oct 9, 2024 •

edited

Loading

mx-psi Oct 9, 2024

TylerHelmuth Oct 15, 2024

bogdandrutu Oct 16, 2024

mx-psi commented Oct 9, 2024

TylerHelmuth commented Oct 9, 2024

bogdandrutu commented Oct 9, 2024

mx-psi commented Oct 9, 2024

[builder] Remove builder default confmap providers #11126

Are you sure you want to change the base?

[builder] Remove builder default confmap providers #11126

Conversation

bogdandrutu commented Sep 10, 2024 • edited Loading

Description

codecov bot commented Sep 10, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mx-psi left a comment

Choose a reason for hiding this comment

bogdandrutu commented Sep 11, 2024

mx-psi commented Sep 11, 2024

bogdandrutu commented Sep 11, 2024

codeboten commented Sep 11, 2024

codeboten commented Sep 11, 2024 • edited Loading

mx-psi commented Sep 11, 2024 • edited Loading

github-actions bot commented Oct 3, 2024

mx-psi commented Oct 3, 2024

bogdandrutu commented Oct 9, 2024

dmitryax commented Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mx-psi commented Oct 9, 2024

TylerHelmuth commented Oct 9, 2024

bogdandrutu commented Oct 9, 2024

mx-psi commented Oct 9, 2024

bogdandrutu commented Sep 10, 2024 •

edited

Loading

codecov bot commented Sep 10, 2024 •

edited

Loading

codeboten commented Sep 11, 2024 •

edited

Loading

mx-psi commented Sep 11, 2024 •

edited

Loading

dmitryax commented Oct 9, 2024 •

edited

Loading