I'm not sure what's going on here. You check here that the single -profile flag has some value, and if it does, then forget about that value and use the one from the iteration 🤔

Basically the -profile value never gets used, and in fact, I'd argue than adding this flag adds confusion to the user, as now they have -profile and -profiles. It seems to me that the single case is the same as -profiles with just one value.

What am I missing?

Pretty on point question. Mainly, naming is hard 😅

It looks like we're not using the -profile flag in dev or production today:

 spec:
  ...
      containers:
        - args:
            - --provider=aws
            - --aws.services=s3

I think the initial intent of the check you see is when doing local development to figure out which profile we should target. In dev/production that should be handled by:

options := []func(*awsconfig.LoadOptions) error{awsconfig.WithEC2IMDSRegion()}

Let me remove the -profile flag as it's not used anymore. I'll build a local image and deploy this into development and make sure it still works 😬

agreed here, it looks like this might've been a misstep. It now appears that we can't even use the flags separately, they must be used together, as specifying -aws.profile will not get used unless there's a profile from -aws.profiles as well, in which case it'll use the value(s) from aws.profiles, but unless you supply aws.profile, it'll never use any config values 😕

I'd suggest something like this, perhaps with a conditional flag to look if aws-profile is defined first rather than a loop through Profiles?

Or better yet, I'd just eliminate the aws.profiles flag entirely and allow aws.profile to be specified multiple times OR eliminate aws.profile and allow aws.profiles to be one or more comma-seperated items.

I've opted to stick with -aws.profiles as a flag, as I think it makes it clear that multiple values can be passed in. @inkel @logyball I've pushed a commit that removes the reference, and appreciate the thoughts/suggestions!

cloudcost-exporter when deployed infers the profile and region from this line:

				options := []func(*awsconfig.LoadOptions) error{awsconfig.WithEC2IMDSRegion()}

I need to be a bit mindful when rolling this change out. If the -profiles isn't set, I've added logic to return an error. So I'll need to likely couple rolling out the wave with adding the -aws.profiles flag to the environment.

agreed here, it looks like this might've been a misstep. It now appears that we can't even use the flags separately, they must be used together, as specifying -aws.profile will not get used unless there's a profile from -aws.profiles as well, in which case it'll use the value(s) from aws.profiles, but unless you supply aws.profile, it'll never use any config values 😕

I'd suggest something like this, perhaps with a conditional flag to look if aws-profile is defined first rather than a loop through Profiles?

Or better yet, I'd just eliminate the aws.profiles flag entirely and allow aws.profile to be specified multiple times OR eliminate aws.profile and allow aws.profiles to be one or more comma-seperated items.

The only thing I'm confused about now is why even have this at all? It's not used anywhere as far as I can tell -

func selectProvider(cfg *config.Config) (provider.Provider, error) {
	switch cfg.Provider {
	case "aws":
		return aws.New(&aws.Config{
			Region:         cfg.Providers.AWS.Region,
			Profiles:       strings.Split(cfg.Providers.AWS.Profiles.String(), ","),
			ScrapeInterval: cfg.Collector.ScrapeInterval,
			Services:       strings.Split(cfg.Providers.AWS.Services.String(), ","),
		})

	case "gcp":
		return google.New(&google.Config{
			ProjectId:       cfg.ProjectID,
			Region:          cfg.Providers.GCP.Region,
			Projects:        cfg.Providers.GCP.Projects.String(),
			DefaultDiscount: cfg.Providers.GCP.DefaultGCSDiscount,
			ScrapeInterval:  cfg.Collector.ScrapeInterval,
			Services:        strings.Split(cfg.Providers.GCP.Services.String(), ","),
		})

	default:
		return nil, fmt.Errorf("unknown provider")
	}
}

Ditto to comment above ^

This doesn't seem to be used at all

👍

Only problem is this doesn't quite work. If you split an empty string, it results in a length one array 😡

Could we either check that:

• cfg.Providers.AWS.Profiles != "" when defined
• move the check to the loop:

case "S3":
			var clients []costexplorer.CostExplorer
			for _, profile := range config.Profiles {
				if profile == "" {
					return nil, fmt.Errorf("invalid profile provided")
				}
				// There are two scenarios:
				// 1. Running locally, the user must pass in a region and profile to use
				// 2. Running within an EC2 instance and the region and profile can be derived
				// I'm going to use the AWS SDK to handle this for me. If the user has provided a region and profile, it will use that.
				// If not, it will use the EC2 instance metadata service to determine the region and credentials.
				// This is the same logic that the AWS CLI uses, so it should be fine.
				options := []func(*awsconfig.LoadOptions) error{awsconfig.WithEC2IMDSRegion()}
				if config.Region != "" {
					options = append(options, awsconfig.WithRegion(config.Region))
				}
				options = append(options, awsconfig.WithSharedConfigProfile(profile))
				ac, err := awsconfig.LoadDefaultConfig(context.Background(), options...)
				if err != nil {
					return nil, err
				}
				client := awscostexplorer.NewFromConfig(ac)
				clients = append(clients, client)
			}

			collector, err := s3.New(config.ScrapeInterval, clients)
			if err != nil {
				return nil, fmt.Errorf("error creating s3 collector: %w", err)
			}
			collectors = append(collectors, collector)```

Note, I have not tested this at length, but anecdotally it has improved the performance of collecting multiple profiles:

// CollectMetrics Collect is the function that will be called by the Prometheus client anytime a scrape is performed.
func (c *Collector) CollectMetrics(ch chan<- prometheus.Metric) float64 {
	c.m.Lock()
	defer c.m.Unlock()
	now := time.Now()
	// :fire: Checking scrape interval is to _mitigate_ expensive API calls to the cost explorer API

	if c.billingData == nil || now.After(c.nextScrape) {
		eg := new(errgroup.Group)
		outputsLock := sync.Mutex{}
		var billingOutputs []*awscostexplorer.GetCostAndUsageOutput
		for _, client := range c.clients {
			eg.Go(func() error {
				endDate := time.Now().AddDate(0, 0, -1)
				// Current assumption is that we're going to pull 30 days worth of billing data
				startDate := endDate.AddDate(0, 0, -30)
				billingData, err := getBillingData(client, startDate, endDate, c.metrics)
				if err != nil {
					return err
				}
				outputsLock.Lock()
				billingOutputs = append(billingOutputs, billingData...)
				outputsLock.Unlock()
				return nil
			})
		}

		err := eg.Wait()
		if err != nil {
			log.Printf("Error getting billing data: %v\n", err)
			return 0
		}
		c.nextScrape = time.Now().Add(c.interval)
		c.metrics.NextScrapeGauge.Set(float64(c.nextScrape.Unix()))
		c.billingData = parseBillingData(billingOutputs)
	}

	exportMetrics(c.billingData, c.metrics)
	return 1.0
}

I can't speak to the validity of the results, but running it this way seemed to speed things up....

Nit GoIsm: remove the empty lines (not the one in line 9, tho).

(suggestion) We probably want to test this, and in order to make it simpler, I'd suggest adding a package variable var ErrNoProfiles = errors.New("no profiles provided") (and BTW, better to use errors.New instead of fmt.Errorf for simple strings). By adding this error then you can use errors.Is in the test.