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

chore: update otelcol core to v0.99.0 #1560

Merged
merged 2 commits into from Apr 25, 2024
Merged

Conversation

kkujawa-sumo
Copy link
Contributor

@kkujawa-sumo kkujawa-sumo commented Apr 24, 2024

chore: update otelcol core to v0.99.0

Deprecations/important changes that impacts update:

  1. [confmap] Add converter and provider settings to confmap.ResolverSettings  open-telemetry/opentelemetry-collector#9516
provider.go:91:15: SA1019: fileprovider.NewWithSettings is deprecated: [v0.99.0] Use NewFactory instead. (staticcheck)
configprovider.go:103:5: SA1019: fileprovider.NewWithSettings is deprecated: [v0.99.0] Use NewFactory instead. (staticcheck)
                                fileprovider.NewWithSettings(settings),
                                ^
configprovider.go:104:5: SA1019: envprovider.NewWithSettings is deprecated: [v0.99.0] Use NewFactory instead. (staticcheck)
                                envprovider.NewWithSettings(settings),
                                ^
configprovider.go:105:5: SA1019: yamlprovider.NewWithSettings is deprecated: [v0.99.0] Use NewFactory instead. (staticcheck)
                                yamlprovider.NewWithSettings(settings),
                                ^
configprovider.go:106:5: SA1019: httpprovider.NewWithSettings is deprecated: [v0.99.0] Use NewFactory instead. (staticcheck)
                                httpprovider.NewWithSettings(settings),
                                ^
configprovider.go:107:5: SA1019: httpsprovider.NewWithSettings is deprecated: [v0.99.0] Use NewFactory instead. (staticcheck)
                                httpsprovider.NewWithSettings(settings),
                                ^
configprovider.go:110:36: SA1019: expandconverter.New is deprecated: [v0.99.0] Use NewFactory instead. (staticcheck)
                        Converters: []confmap.Converter{expandconverter.New(confmap.ConverterSettings{})},
                                                        ^
configprovider.go:122:36: SA1019: expandconverter.New is deprecated: [v0.99.0] Use NewFactory instead. (staticcheck)
                        Converters: []confmap.Converter{expandconverter.New(confmap.ConverterSettings{})},
                                                        ^
  1. [confighttp] deprecate ToClientContext, ToServerContext, ToListenerContext open-telemetry/opentelemetry-collector#9944
exporter.go:407:17: SA1019: httpSettings.ToClientContext is deprecated: [v0.99.0] Use ToClient instead. (staticcheck)
        client, err := httpSettings.ToClientContext(ctx, se.host, component.TelemetrySettings{})
                       ^
sender_test.go:89:17: SA1019: httpSettings.ToClientContext is deprecated: [v0.99.0] Use ToClient instead. (staticcheck)
        client, err := httpSettings.ToClientContext(context.TODO(), host, component.TelemetrySettings{})
  1. [configtls] Add context.Context to public functions open-telemetry/opentelemetry-collector#9813
config.go:1: : # github.com/SumoLogic/sumologic-otel-collector/pkg/exporter/syslogexporter [github.com/SumoLogic/sumologic-otel-collector/pkg/exporter/syslogexporter.test]
./exporter.go:39:20: not enough arguments in call to cfg.TLSSetting.LoadTLSConfig
        have ()
        want (context.Context) (typecheck)

@kkujawa-sumo kkujawa-sumo force-pushed the kkujawa_update_ot_core branch 2 times, most recently from 0394541 to 0da675c Compare April 25, 2024 07:49
@kkujawa-sumo kkujawa-sumo marked this pull request as ready for review April 25, 2024 08:21
@kkujawa-sumo kkujawa-sumo requested a review from a team as a code owner April 25, 2024 08:21
envprovider.NewFactory(),
yamlprovider.NewFactory(),
httpprovider.NewFactory(),
httpsprovider.NewFactory(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you compare with linux collector? I see we are missing globprovider here

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'm not sure if we need to have collector_windows.go in our repo, main_windows.go has this form:

// Code generated by "go.opentelemetry.io/collector/cmd/builder". DO NOT EDIT.

//go:build windows

package main

import (
	"errors"
	"fmt"
	"golang.org/x/sys/windows"
	"golang.org/x/sys/windows/svc"
	"go.opentelemetry.io/collector/otelcol"
)

func run(params otelcol.CollectorSettings) error {
	// No need to supply service name when startup is invoked through
	// the Service Control Manager directly.
	err := UseCustomConfigProvider(&params)
	if err != nil {
		return err
	}
	cmd := otelcol.NewCommand(params)
	// this is only here so that the flag can be recognized by the upstream parser and displayed in help
	cmd.Flags().StringVarP(&opAmpConfig, "remote-config", "", "", "path to opamp config file")
	if err := svc.Run("", NewSvcHandler(params)); err != nil {
		if errors.Is(err, windows.ERROR_FAILED_SERVICE_CONTROLLER_CONNECT) {
			// Per https://learn.microsoft.com/en-us/windows/win32/api/winsvc/nf-winsvc-startservicectrldispatchera#return-value
			// this means that the process is not running as a service, so run interactively.
			return runInteractive(params)
		}

		return fmt.Errorf("failed to start collector server: %w", err)
	}

	return nil
}

UseCustomConfigProvider uses this NewConfigProviderSettings which has globprovider 🤷

We have test for globprovider - here and tests are executed on windows

Copy link
Contributor Author

Choose a reason for hiding this comment

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

issue for checking if we need to have collector_windows.go #1565

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this code is used right now. For now, it's reasonable to add globprovider here though, imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I'll add globprovider here

@kkujawa-sumo kkujawa-sumo merged commit 8657413 into main Apr 25, 2024
29 checks passed
@kkujawa-sumo kkujawa-sumo deleted the kkujawa_update_ot_core branch April 25, 2024 11:40
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

3 participants