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

[Furisto]

Description

Add team and project to ws-manager logs and traces.

Logs:

Traces:

Related Issue(s)

Fixes #9374

How to test

• Create a team and within that team a project
• Open a workspace within the project
• Check that the pod contains project and team labels
• Search for team id/projectId in logs explorer
• Search for team id/projectId in honeycomb

Release Notes

NONE

[Furisto]

/werft run

👍 started the job as gitpod-build-fo-log-context.5
(with .werft/ from main)

[sagor999]

nit: why not to just add it above into labels map as:
wsk8s.ProjectLabel: req.Metadata.Project ?
If it is empty, it will be an empty label. Which I think is fine.

[jenting]

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,
	}
}

[Furisto]

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.

[jenting]

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)

[geropl]

Shouldn't these be optional?

[Furisto]

That's a good question! Here is how I understand it: In contrast to proto2, proto3 uses no presence semantics as default (i.e. it just does not serialize values that are not present) and does not use explicit presence semantics (i.e. it stores if a field has explicitly been set to a value).

Tagging the fields with optional would switch it to explicit presence semantics. Explicit presence therefore increases message size, even when no value is present, because of the tracking requirement. In return additional methods are generated that allow you to check if a value has been set. I do not foresee a need to do this, as with no presence semantics we get an empty string for team/project which should be good enough for us. What do you think?

[geropl]

I do not foresee a need to do this, as with no presence semantics we get an empty string for team/project which should be good enough for us. What do you think?

Sometimes optional is nice for readability bc a) you get the has_ accessor and b) for making it explicit in the proto file. (Or if you need the extra bit in case you need the default value in the value domain - not the case here),
But searching our repo we mention optional mostly on comments, so fine with not using it! 👍 A comment would be nice, though. 🙂

[geropl]

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! 🧡

[geropl]

Code LGTM and works! 👍

/hold in case you want to address this comment

[Furisto]

@aledbf You are configured as one of the reviewers. Can you please take a look?

[Furisto]

/unhold