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

Add reported name field to Source, deprecate the odigos.io/reported-name annotation #2298

Merged
merged 27 commits into from
Jan 27, 2025
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
adefb72
Add reported name field to Source, deprecate the odigos.io/reported-n…
RonFed Jan 23, 2025
3f53341
Add migration to the reported name annotation
RonFed Jan 23, 2025
7f9035a
update e2e tests
RonFed Jan 23, 2025
ca65ba3
Improve error message
RonFed Jan 23, 2025
92cf980
change pods webhook to get service name from the sources
RonFed Jan 23, 2025
e52dcc7
update api docs
RonFed Jan 23, 2025
c3f11bf
update sources test generations
RonFed Jan 23, 2025
df8de83
Set default service name in pods webhook
RonFed Jan 23, 2025
e6fb223
Update source e2e test
RonFed Jan 23, 2025
c36ef9b
Update workload-lifecycle test
RonFed Jan 24, 2025
de79e00
mark reported name as optional
RonFed Jan 24, 2025
a1ed0fb
Add case for source-webhooks test
RonFed Jan 24, 2025
5552e64
Update cli-upgrade test
RonFed Jan 24, 2025
37ab074
Merge branch 'main' into reported_name_to_source
RonFed Jan 24, 2025
d18867c
Update instrumentor/sources_webhooks.go
RonFed Jan 25, 2025
345480f
Rename ReportedName to OtelServiceName
RonFed Jan 25, 2025
3207db5
Add logging for migration from reported name annotation
RonFed Jan 25, 2025
d273ec7
Simplify source controller under instrumentationconfig
RonFed Jan 25, 2025
3a01f46
address code review comments
RonFed Jan 26, 2025
b549dff
Update docs
RonFed Jan 26, 2025
b62f750
Merge branch 'main' into reported_name_to_source
RonFed Jan 26, 2025
cdcc685
update Source update docs
RonFed Jan 26, 2025
57bf3a9
Update Source update doc
RonFed Jan 27, 2025
ff263b6
Migrate from annotation only if it had a non empty value
RonFed Jan 27, 2025
c81c94b
remove unused scheme
RonFed Jan 27, 2025
e46f8ce
Merge branch 'main' into reported_name_to_source
RonFed Jan 27, 2025
fa34c20
Merge branch 'main' into reported_name_to_source
RonFed Jan 27, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions api/config/crd/bases/odigos.io_sources.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,12 @@ spec:
disableInstrumentation:
description: DisableInstrumentation excludes this workload from auto-instrumentation.
type: boolean
reportedName:
description: |-
ReportedName determines the "service.name" resource attribute which will be reported by the instrumentations of this source.
If not set, the workload name will be used.
It is not valid for namespace sources.
type: string
workload:
description: |-
Workload represents the workload or namespace to be instrumented.
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions api/odigos/v1alpha1/source_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ type SourceSpec struct {
// DisableInstrumentation excludes this workload from auto-instrumentation.
// +kubebuilder:validation:Optional
DisableInstrumentation bool `json:"disableInstrumentation,omitempty"`
// ReportedName determines the "service.name" resource attribute which will be reported by the instrumentations of this source.
// If not set, the workload name will be used.
// It is not valid for namespace sources.
ReportedName string `json:"reportedName,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

needs // +kubebuilder:validation:Optional (generated docs show it as required)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks like we have Required for all of our fields and // +kubebuilder:validation:Optional is not doing anything.
Adding // +optional did the trick, which seems to be what the genref looks for

I can do a follow up PR adding it to all our fields. (and maybe remove // +kubebuilder:validation:Optional which seems to not do anything?)

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, weird. I think that's a bug in the genref, they should definitely be parsing +kubebuilder tags too

A follow up to add // +optional would be good. I think it will only affect the docs, because the controller-gen scripts should be respecting either tag. Meaning, I think our CRDs are accurate

RonFed marked this conversation as resolved.
Show resolved Hide resolved
}

type SourceStatus struct {
Expand Down
28 changes: 15 additions & 13 deletions common/consts/consts.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,21 @@ import (
)

const (
CurrentNamespaceEnvVar = "CURRENT_NS"
OdigosVersionEnvVarName = "ODIGOS_VERSION"
OdigosTierEnvVarName = "ODIGOS_TIER"
DefaultOdigosNamespace = "odigos-system"
OdigosConfigurationName = "odigos-config"
OdigosEffectiveConfigName = "effective-config"
OdigosConfigurationFileName = "config.yaml"
OTLPPort = 4317
OTLPHttpPort = 4318
PprofOdigosPort = 6060
OdigosInstrumentationLabel = "odigos-instrumentation"
InstrumentationEnabled = "enabled"
InstrumentationDisabled = "disabled"
CurrentNamespaceEnvVar = "CURRENT_NS"
OdigosVersionEnvVarName = "ODIGOS_VERSION"
OdigosTierEnvVarName = "ODIGOS_TIER"
DefaultOdigosNamespace = "odigos-system"
OdigosConfigurationName = "odigos-config"
OdigosEffectiveConfigName = "effective-config"
OdigosConfigurationFileName = "config.yaml"
OTLPPort = 4317
OTLPHttpPort = 4318
PprofOdigosPort = 6060
OdigosInstrumentationLabel = "odigos-instrumentation"
InstrumentationEnabled = "enabled"
InstrumentationDisabled = "disabled"

// Deprecated: reported name is set via the Source CR.
OdigosReportedNameAnnotation = "odigos.io/reported-name"
RolloutTriggerAnnotation = "rollout-trigger"

Expand Down
13 changes: 13 additions & 0 deletions docs/api-reference/odigos.io.v1alpha1.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -2489,6 +2489,19 @@ This field is required upon creation and cannot be modified.</p>
<p>DisableInstrumentation excludes this workload from auto-instrumentation.</p>
</td>
</tr>
<tr>
<td>
<code>reportedName</code> <B>[Required]</B>
</td>
<td>
<code>string</code>
</td>
<td>
<p>ReportedName determines the &quot;service.name&quot; resource attribute which will be reported by the instrumentations of this source.
If not set, the workload name will be used.
It is not valid for namespace sources.</p>
</td>
</tr>
</tbody>
</table>

Expand Down
6 changes: 6 additions & 0 deletions helm/odigos/templates/crds/odigos.io_sources.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,12 @@ spec:
disableInstrumentation:
description: DisableInstrumentation excludes this workload from auto-instrumentation.
type: boolean
reportedName:
description: |-
ReportedName determines the "service.name" resource attribute which will be reported by the instrumentations of this source.
If not set, the workload name will be used.
It is not valid for namespace sources.
type: string
workload:
description: |-
Workload represents the workload or namespace to be instrumented.
Expand Down
17 changes: 1 addition & 16 deletions instrumentor/controllers/instrumentationconfig/common.go
Original file line number Diff line number Diff line change
@@ -1,18 +1,14 @@
package instrumentationconfig

import (
"context"
"fmt"

odigosv1alpha1 "github.com/odigos-io/odigos/api/odigos/v1alpha1"
"github.com/odigos-io/odigos/api/odigos/v1alpha1/instrumentationrules"
"github.com/odigos-io/odigos/common"
"github.com/odigos-io/odigos/instrumentor/controllers/utils"
"github.com/odigos-io/odigos/k8sutils/pkg/workload"
"sigs.k8s.io/controller-runtime/pkg/client"
)

func updateInstrumentationConfigForWorkload(ic *odigosv1alpha1.InstrumentationConfig, rules *odigosv1alpha1.InstrumentationRuleList, serviceName string) error {
func updateInstrumentationConfigForWorkload(ic *odigosv1alpha1.InstrumentationConfig, rules *odigosv1alpha1.InstrumentationRuleList) error {

workloadName, workloadKind, err := workload.ExtractWorkloadInfoFromRuntimeObjectName(ic.Name)
if err != nil {
Expand All @@ -24,8 +20,6 @@ func updateInstrumentationConfigForWorkload(ic *odigosv1alpha1.InstrumentationCo
Kind: workloadKind,
}

ic.Spec.ServiceName = serviceName

sdkConfigs := make([]odigosv1alpha1.SdkConfig, 0, len(ic.Status.RuntimeDetailsByContainer))

// create an empty sdk config for each detected programming language
Expand Down Expand Up @@ -283,12 +277,3 @@ func boolPtr(b bool) *bool {
return &b
}

func resolveServiceName(ctx context.Context, k8sClient client.Client, workloadName string, namespace string, kind workload.WorkloadKind) (string, error) {
objectKey := client.ObjectKey{Name: workloadName, Namespace: namespace}
obj, err := workload.GetWorkloadObject(ctx, objectKey, kind, k8sClient)

if err != nil {
return "", fmt.Errorf("failed to get workload object to resolve reported service name annotation. will use fallback service name: %w", err)
}
return workload.ExtractServiceNameFromAnnotations(obj.GetAnnotations(), workloadName), nil
}
24 changes: 12 additions & 12 deletions instrumentor/controllers/instrumentationconfig/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func TestUpdateInstrumentationConfigForWorkload_SingleLanguage(t *testing.T) {
},
}
rules := &odigosv1.InstrumentationRuleList{}
err := updateInstrumentationConfigForWorkload(&ic, rules, "service-name")
err := updateInstrumentationConfigForWorkload(&ic, rules)
if err != nil {
t.Errorf("Expected nil error, got %v", err)
}
Expand Down Expand Up @@ -60,7 +60,7 @@ func TestUpdateInstrumentationConfigForWorkload_MultipleLanguages(t *testing.T)
},
}
rules := &odigosv1.InstrumentationRuleList{}
err := updateInstrumentationConfigForWorkload(&ic, rules, "service-name")
err := updateInstrumentationConfigForWorkload(&ic, rules)
if err != nil {
t.Errorf("Expected nil error, got %v", err)
}
Expand Down Expand Up @@ -101,7 +101,7 @@ func TestUpdateInstrumentationConfigForWorkload_IgnoreUnknownLanguage(t *testing
},
}
rules := &odigosv1.InstrumentationRuleList{}
err := updateInstrumentationConfigForWorkload(&ic, rules, "service-name")
err := updateInstrumentationConfigForWorkload(&ic, rules)
if err != nil {
t.Errorf("Expected nil error, got %v", err)
}
Expand All @@ -126,7 +126,7 @@ func TestUpdateInstrumentationConfigForWorkload_NoLanguages(t *testing.T) {
},
}
rules := &odigosv1.InstrumentationRuleList{}
err := updateInstrumentationConfigForWorkload(&ic, rules, "service-name")
err := updateInstrumentationConfigForWorkload(&ic, rules)
if err != nil {
t.Errorf("Expected nil error, got %v", err)
}
Expand Down Expand Up @@ -156,7 +156,7 @@ func TestUpdateInstrumentationConfigForWorkload_SameLanguageMultipleContainers(t
},
}
rules := &odigosv1.InstrumentationRuleList{}
err := updateInstrumentationConfigForWorkload(&ic, rules, "service-name")
err := updateInstrumentationConfigForWorkload(&ic, rules)
if err != nil {
t.Errorf("Expected nil error, got %v", err)
}
Expand Down Expand Up @@ -199,7 +199,7 @@ func TestUpdateInstrumentationConfigForWorkload_SingleMatchingRule(t *testing.T)
},
},
}
err := updateInstrumentationConfigForWorkload(&ic, rules, "service-name")
err := updateInstrumentationConfigForWorkload(&ic, rules)
if err != nil {
t.Errorf("Expected nil error, got %v", err)
}
Expand Down Expand Up @@ -259,7 +259,7 @@ func TestUpdateInstrumentationConfigForWorkload_InWorkloadList(t *testing.T) {
},
}

err := updateInstrumentationConfigForWorkload(&ic, rules, "service-name")
err := updateInstrumentationConfigForWorkload(&ic, rules)
if err != nil {
t.Errorf("Expected nil error, got %v", err)
}
Expand Down Expand Up @@ -309,7 +309,7 @@ func TestUpdateInstrumentationConfigForWorkload_NotInWorkloadList(t *testing.T)
},
}

err := updateInstrumentationConfigForWorkload(&ic, rules, "service-name")
err := updateInstrumentationConfigForWorkload(&ic, rules)
if err != nil {
t.Errorf("Expected nil error, got %v", err)
}
Expand Down Expand Up @@ -354,7 +354,7 @@ func TestUpdateInstrumentationConfigForWorkload_DisabledRule(t *testing.T) {
},
}

err := updateInstrumentationConfigForWorkload(&ic, rules, "service-name")
err := updateInstrumentationConfigForWorkload(&ic, rules)
if err != nil {
t.Errorf("Expected nil error, got %v", err)
}
Expand Down Expand Up @@ -411,7 +411,7 @@ func TestUpdateInstrumentationConfigForWorkload_MultipleDefaultRules(t *testing.
},
}

err := updateInstrumentationConfigForWorkload(&ic, rules, "service-name")
err := updateInstrumentationConfigForWorkload(&ic, rules)
if err != nil {
t.Errorf("Expected nil error, got %v", err)
}
Expand Down Expand Up @@ -496,7 +496,7 @@ func TestUpdateInstrumentationConfigForWorkload_RuleForLibrary(t *testing.T) {
},
}

err := updateInstrumentationConfigForWorkload(&ic, rules, "service-name")
err := updateInstrumentationConfigForWorkload(&ic, rules)
if err != nil {
t.Errorf("Expected nil error, got %v", err)
}
Expand Down Expand Up @@ -553,7 +553,7 @@ func TestUpdateInstrumentationConfigForWorkload_LibraryRuleOtherLanguage(t *test
},
}

err := updateInstrumentationConfigForWorkload(&ic, rules, "service-name")
err := updateInstrumentationConfigForWorkload(&ic, rules)
if err != nil {
t.Errorf("Expected nil error, got %v", err)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (

odigosv1 "github.com/odigos-io/odigos/api/odigos/v1alpha1"
"github.com/odigos-io/odigos/k8sutils/pkg/utils"
"github.com/odigos-io/odigos/k8sutils/pkg/workload"
"k8s.io/apimachinery/pkg/runtime"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand All @@ -42,24 +41,13 @@ func (r *InstrumentationConfigReconciler) Reconcile(ctx context.Context, req ctr
return ctrl.Result{}, client.IgnoreNotFound(err)
}

workloadName, workloadKind, err := workload.ExtractWorkloadInfoFromRuntimeObjectName(ic.Name)
if err != nil {
return ctrl.Result{}, err
}

serviceName, err := resolveServiceName(ctx, r.Client, workloadName, ic.Namespace, workloadKind)
if err != nil {
logger.Error(err, "Failed to resolve service name", "workload", workloadName, "kind", workloadKind)
return ctrl.Result{}, err
}

instrumentationRules := &odigosv1.InstrumentationRuleList{}
err = r.Client.List(ctx, instrumentationRules)
if client.IgnoreNotFound(err) != nil {
return ctrl.Result{}, err
}

err = updateInstrumentationConfigForWorkload(&ic, instrumentationRules, serviceName)
err = updateInstrumentationConfigForWorkload(&ic, instrumentationRules)
if err != nil {
return ctrl.Result{}, err
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"

odigosv1alpha1 "github.com/odigos-io/odigos/api/odigos/v1alpha1"
"github.com/odigos-io/odigos/k8sutils/pkg/workload"
"k8s.io/apimachinery/pkg/runtime"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -33,18 +32,7 @@ func (r *InstrumentationRuleReconciler) Reconcile(ctx context.Context, req ctrl.

for _, ic := range instrumentationConfigs.Items {
currIc := ic
workloadName, workloadKind, err := workload.ExtractWorkloadInfoFromRuntimeObjectName(ic.Name)
if err != nil {
return ctrl.Result{}, err
}

serviceName, err := resolveServiceName(ctx, r.Client, workloadName, ic.Namespace, workloadKind)
if err != nil {
logger.Error(err, "error resolving service name", "workload", ic.Name)
continue
}

err = updateInstrumentationConfigForWorkload(&currIc, instrumentationRules, serviceName)
err = updateInstrumentationConfigForWorkload(&currIc, instrumentationRules)
if err != nil {
logger.Error(err, "error updating instrumentation config", "workload", ic.Name)
continue
Expand Down
36 changes: 4 additions & 32 deletions instrumentor/controllers/instrumentationconfig/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package instrumentationconfig
import (
odigosv1alpha1 "github.com/odigos-io/odigos/api/odigos/v1alpha1"
instrumentorpredicate "github.com/odigos-io/odigos/instrumentor/controllers/utils/predicates"
appsv1 "k8s.io/api/apps/v1"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/builder"
)
Expand Down Expand Up @@ -35,39 +34,12 @@ func SetupWithManager(mgr ctrl.Manager) error {
return err
}

// Watch for Deployment changes using DeploymentReconciler
// Watch for Source changes to update InstrumentationConfig
if err := builder.
ControllerManagedBy(mgr).
Named("instrumentor-instrumentationconfig-deployment").
For(&appsv1.Deployment{}).
WithEventFilter(workloadReportedNameAnnotationChanged{}).
Complete(&DeploymentReconciler{
Client: mgr.GetClient(),
Scheme: mgr.GetScheme(),
}); err != nil {
return err
}

// Watch for StatefulSet changes using StatefulSetReconciler
if err := builder.
ControllerManagedBy(mgr).
Named("instrumentor-instrumentationconfig-statefulset").
For(&appsv1.StatefulSet{}).
WithEventFilter(workloadReportedNameAnnotationChanged{}).
Complete(&StatefulSetReconciler{
Client: mgr.GetClient(),
Scheme: mgr.GetScheme(),
}); err != nil {
return err
}

// Watch for DaemonSet changes using DaemonSetReconciler
if err := builder.
ControllerManagedBy(mgr).
Named("instrumentor-instrumentationconfig-daemonset").
For(&appsv1.DaemonSet{}).
WithEventFilter(workloadReportedNameAnnotationChanged{}).
Complete(&DaemonSetReconciler{
Named("instrumentor-instrumentationconfig-source").
For(&odigosv1alpha1.Source{}).
Complete(&SourceReconciler{
damemi marked this conversation as resolved.
Show resolved Hide resolved
Client: mgr.GetClient(),
Scheme: mgr.GetScheme(),
}); err != nil {
Expand Down
Loading
Loading