Skip to content

[ws-manager] Add team and project to logs and traces #10632

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jun 15, 2022
Merged

[ws-manager] Add team and project to logs and traces #10632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
569caf1
Add team and project to workspace request
Furisto Jun 10, 2022
1a14707
Extend ws-manager tracing with team and project
Furisto Jun 10, 2022
1a898c2
Always apply labels and update tests
Furisto Jun 10, 2022
ab093c1
Make some workspace metadata properties optional
Furisto Jun 15, 2022
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
10 changes: 9 additions & 1 deletion components/common-go/kubernetes/kubernetes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ const (
// MetaIDLabel is the label of the workspace meta ID (just workspace ID outside of wsman)
MetaIDLabel = "metaID"

// ProjectLabel is the label for the workspace's project
ProjectLabel = "project"

// TeamLabel is the label for the workspace's team
TeamLabel = "team"

// TypeLabel marks the workspace type
TypeLabel = "workspaceType"

Expand Down Expand Up @@ -71,7 +77,9 @@ func GetOWIFromObject(pod *metav1.ObjectMeta) logrus.Fields {
owner := pod.Labels[OwnerLabel]
workspace := pod.Labels[MetaIDLabel]
instance := pod.Labels[WorkspaceIDLabel]
return log.OWI(owner, workspace, instance)
project := pod.Labels[ProjectLabel]
team := pod.Labels[TeamLabel]
return log.LogContext(owner, workspace, instance, project, team)
}

// UnlimitedRateLimiter implements an empty, unlimited flowcontrol.RateLimiter
Expand Down
33 changes: 33 additions & 0 deletions components/common-go/log/log.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ const (
WorkspaceField = "workspaceId"
// InstanceField is the log field name of a workspace instance ID
InstanceField = "instanceId"
// ProjectField is the log field name of the project
ProjectField = "projectId"
// TeamField is the log field name of the team
TeamField = "teamId"
)

// OWI builds a structure meant for logrus which contains the owner, workspace and instance.
Expand All @@ -36,6 +40,35 @@ func OWI(owner, workspace, instance string) log.Fields {
}
}

// LogContext builds a structure meant for logrus which contains the owner, workspace and instance.
// Beware that this refers to the terminology outside of wsman which maps like:
// owner = owner, workspace = metaID, instance = workspaceID
func LogContext(owner, workspace, instance, project, team string) log.Fields {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could extend this function

gitpod/components/common-go/log/log.go

Line 31 in e1b335c

func OWI(owner, workspace, instance string) log.Fields {
to have the project and team as well.

Or to create another function

func PT(project, team string) log.Fields {
	return log.Fields{
		Project: project,
		Team: team,
	}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

The OWI function is used across many places where we do not currently log the team/project and Go does not support optional parameters, so that would require adapting all usages. I do not think that is a good trade-off.
Regarding your second suggestion we need all 5 fields, only project and team is not enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, my idea was has a another function like this

func PT(project, team string) log.Fields {
	return log.Fields{
		Project: project,
		Team: team,
	}
}

So, the caller would be

owi := log.OWI(req.Metadata.Owner, req.Metadata.MetaId, req.Id)
pt := log.PT(project, team)
clog := log.WithFields(owi).WithFields(pt)

logFields := log.Fields{}

if owner != "" {
logFields[OwnerField] = owner
}

if workspace != "" {
logFields[WorkspaceField] = workspace
}

if instance != "" {
logFields[InstanceField] = instance
}

if project != "" {
logFields[ProjectField] = project
}

if team != "" {
logFields[TeamField] = team
}

return logFields
}

// ServiceContext is the shape required for proper error logging in the GCP context.
// See https://cloud.google.com/error-reporting/reference/rest/v1beta1/ServiceContext
// Note that we musn't set resourceType for reporting errors.
Expand Down
2 changes: 1 addition & 1 deletion components/common-go/tracing/tracing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ func FromContext(ctx context.Context, name string) (opentracing.Span, context.Co

// ApplyOWI sets the owner, workspace and instance tags on a span
func ApplyOWI(span opentracing.Span, owi logrus.Fields) {
for _, k := range []string{log.OwnerField, log.WorkspaceField, log.InstanceField} {
for _, k := range []string{log.OwnerField, log.WorkspaceField, log.InstanceField, log.ProjectField, log.TeamField} {
Copy link
Member

@geropl geropl Jun 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Not necessarily in-scope for this PR, but: We started writing up some naming conventions for tracing, incl. for OWI. (just extended it for projectId/teamId 😇 ).
Would be awesome to use on common scheme across all components! 🧡

val, ok := owi[k]
if !ok {
continue
Expand Down
19 changes: 16 additions & 3 deletions components/server/src/workspace/workspace-starter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,9 +405,7 @@ export class WorkspaceStarter {
);

// create start workspace request
const metadata = new WorkspaceMetadata();
metadata.setOwner(workspace.ownerId);
metadata.setMetaId(workspace.id);
const metadata = await this.createMetadata(workspace);
const startRequest = new StartWorkspaceRequest();
startRequest.setId(instance.id);
startRequest.setMetadata(metadata);
Expand Down Expand Up @@ -484,6 +482,21 @@ export class WorkspaceStarter {
}
}

protected async createMetadata(workspace: Workspace): Promise<WorkspaceMetadata> {
let metadata = new WorkspaceMetadata();
metadata.setOwner(workspace.ownerId);
metadata.setMetaId(workspace.id);
if (workspace.projectId) {
metadata.setProject(workspace.projectId);
let project = await this.projectDB.findProjectById(workspace.projectId);
if (project && project.teamId) {
metadata.setTeam(project.teamId);
}
}

return metadata;
}

protected async tryStartOnCluster(
ctx: TraceContext,
startRequest: StartWorkspaceRequest,
Expand Down
6 changes: 6 additions & 0 deletions components/ws-manager-api/core.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,12 @@ message WorkspaceMetadata {
// Annotations are key/value pairs that gets attached to the workspace.
// This is primarily intended for annotating headless workspace loads.
map<string, string> annotations = 4;

// team the workspace belongs to, if the workspace is not associated with a team, this property will be empty
optional string team = 5;

// project the workspace belongs to, if the workspace is not associated with a project, this property will be empty
optional string project = 6;
}

// WorkspaceRuntimeInfo details the workspace's runtime, e.g. executing system, node other information
Expand Down
Loading