Feature/gen 2030 group sources to destination

[alonkeyval]

No description provided.

[damemi]

Can you add an e2e test for this? You should be able to add it to tests/e2e/source to show:

• Workload Source with odigos.io/group- label works with grouping
• Namespace Source with odigos.io/group- label includes all the workloads in the namespace
• Workload Source with disableInstrumentation=true excludes that Source from the grouping (? not sure what the behavior should be here)

[damemi]

how will this work with Namespace Sources? You can create one Source to instrument an entire namespace, so won't you need to do some sort of linking between the workload and the namespace source?

[alonkeyval]

Currently we rely on the source crd to represent a group. What do you think about creating the source crd if it doesn't exist (because the source is represented by source crd from namespace kind) once the UI creates a group?

in this case we will have the source crd that represent the workload and one more for the namespace

[damemi]

We should minimize how often we create Sources on behalf of the user, since it's supposed to be a user-created object. Also, even the UI doesn't create Namespace Sources so if there is one it means the user likely intentionally created it instead of using Workload Sources.

I think it would be better here to handle a Namespace Source as a special case, where you loop through all the workloads in the namespace and check if they should inherit the group from the namespace (like we do in the Instrumentor).

The next question is what if a workload Source already exists with a different group than the namespace Source? Should the workload be part of both groups? Or should the workload Source take priority? I think to begin with, it should take priority over the namespace.

So, when you see a Namespace Source:

1. List all other Sources in that Namespace to check for different groups or excluded workloads
2. Remember any workloads with a different group or excluded
3. List through all the deployments/statefulsets/daemonsets in the namespace
4. If the workload isn't in the list from earlier, add it to the group in the Namespace Source

The pseudo-go would look like this:

matchedSources := fetchSourcesByGroups(ctx, kubeClient, dest.Spec.SourceSelector.Groups, logger)
for _, source := range matchedSources {
  if source.Spec.Workload.Kind == workload.WorkloadKindNamespace {
    excludedWorkloads := make(map[string]struct{}{})
    // exclude disabled Sources or Sources with a different group
    nsSources := &odigosv1.SourceList{}
    err := kubeClient.List(ctx, sourceList, client.InNamespace(source.Spec.Workload.Namespace))
    if err != nil {
      continue
    }
    for _, nsSource := range nsSources.Items {
      val, exists := nsSource.Labels["odigos.io/source-group"]
      // if the nsSource has a group label that's different from the original matchedSource, or the Source is disabled, exclude it
      if (exists && val != source.Labels["odigos.io/source-group"]) || !nsSource.Spec.DisableInstrumentation {
        excludedWorkloads[fmt.Sprintf("%s/%s/%s", source.Spec.Workload.Namespace, source.Spec.Workload.Name, source.Spec.Workload.Kind] = struct{}{}
      }

    // get workload keys for non-excluded workloads in the namespace
    deployments := &v1.DeploymentList{}
    err := kubeClient.List(ctx, deployments, client.InNamespace(source.Spec.Workload.Namespace))
      if err != nil {
        continue
      } 
    }
    
    for _, dep := range deployments.Items {
      key := fmt.Sprintf("%s/%s/%s", deployment.GetNamespace(), deployment.GetName(), workload.WorkloadKindDeployment)
      if _, exists := excludedWorkloads[key]; !exists {
        matchConditions = append(matchConditions, key)
      }
    }
    // repeat for statefulsets and daemonsets

  } else { //not a namespace source, follow the flow you have now
    key := fmt.Sprintf("%s/%s/%s", source.Spec.Workload.Namespace, source.Spec.Workload.Name, source.Spec.Workload.Kind)
    matchConditions = append(matchConditions, key)
  }
}

This should be a test case too.

Technically, you probably don't even need to handle any workload Sources with disableInstrumentation: true, because they won't be sending instrumentation anyway. But I guess it doesn't hurt here.

[tamirdavid1]

In general I think that we should do it with connector as discussed

[tamirdavid1]

I think because we will have lots of logic around the sources better to move it under a dedicated pkg under k8sutils e.g sources

[blumamir]

Looks good 👍
Added few comments most are nit

[blumamir]

Why is this needed?

[blumamir]

since you make the decision based on the resource attributes only, you can remove the spans (metrics/logs) iterations and simply remove or keep the entire scopeSpans

[blumamir]

prefer to use a boolean value to indicate if the attribute is found, instead of relying on empty value

[blumamir]

same here, return "found" boolean to indicate this

[blumamir]

we usually write kind before name. e.g. deployment-foo and not foo-deployment

Suggested change

	key := fmt.Sprintf("%s/%s/%s", source.Spec.Workload.Namespace, source.Spec.Workload.Name, source.Spec.Workload.Kind)
	key := fmt.Sprintf("%s/%s/%s", source.Spec.Workload.Namespace, source.Spec.Workload.Kind, source.Spec.Workload.Name)

[blumamir]

this is not part of the config. I think it should go directly as a member of the filterProcessor

[damemi]

What about making the label name constant, and the group is the value of the label? So instead of

odigos.io/group-foo: true

it's

odigos.io/source-group: foo

Labels are meant to hold variable string values