chore: adds org lookup type to instrumentation data

[j-luong]

Description

This PR adds some extra instrumentation data to how we perform org lookups.

Checklist

• Tests added and all succeed (make test)
• Regenerated mocks, etc. (make generate)
• Linted (make lint)
• Test your changes work for the CLI
  1. Clone / pull the latest CLI main.
  2. Run go get github.com/snyk/go-application-framework@YOUR_LATEST_GAF_COMMIT in the cliv2 directory.
     • Tip: for local testing, you can uncomment the line near the bottom of the CLI's go.mod to point to your local GAF code.
  3. Run go mod tidy in the cliv2 directory.
  4. Run the CLI tests and do any required manual testing.
  5. Open a PR in the CLI repo now with the go.mod and go.sum changes.
  • Once this PR is merged, repeat these steps, but pointing to the latest GAF commit on main and update your CLI PR.

[snyk-io]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[snyk-io]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[PeterSchafer]

Suggestion: maybe be a bit more explicit and self explaining in the data. Consider a case that someone will just have the instrumentation data, will it be self explaining?

[PeterSchafer]

thinking of the more complete picture (name: value)

• organization-id-origin: defaultid-failed-slugname-lookup
• organization-id-origin: defaultid-not-user-supplied
• organization-id-origin: slugname-lookup
• organization-id-origin: user-supplied

I don't like my proposal but maybe still food for thought?

[j-luong]

made an opinionated change, let me know if it works

[j-luong]

context: a command like snyk test --org=<INVALID_UUID> will result in the orgLookupkey value being orgid instead of invalid

[j-luong]

context: calling this before we finish initialising our engine means our analytics is not initialised and will lead to it being nil when trying to use it in our config.DefaultValueFunction.

The consequence is that we have to set this where we use GAF (e.g. CLI) instead.

Possibly a controversial change but, without this, we won't get analytics implemented in the rest of the PR. Additionally, the argument could be made that none of these "Sets" should be a GAF concern.

[snyk-pr-review-bot]

PR Reviewer Guide 🔍

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Telemetry Regression 🟠 [major] Removing the call to a.SetOrg() in initAnalytics() prevents the organization ID from being associated with the global analytics object. The reference code in pkg/analytics/analytics.go shows that the org query parameter is only added to the analytics POST request if this field is populated. Removing this call will cause all background telemetry to be sent without an organization identifier in the URL, likely breaking server-side attribution and filtering that relies on the query parameter. func (e *EngineImpl) initAnalytics() analytics.Analytics { a := analytics.New() a.SetIntegration(e.config.GetString(configuration.INTEGRATION_NAME), e.config.GetString(configuration.INTEGRATION_VERSION)) a.SetApiUrl(e.config.GetString(configuration.API_URL)) a.SetClient(func() *http.Client { return e.networkAccess.GetHttpClient() }) Instrumentation Data Loss 🟡 [minor] The newInstrumentationRecorder helper silently discards telemetry data if the engine's analytics object has not yet been initialized. In pkg/workflow/engineimpl.go, Init() executes extension initializers before calling initAnalytics(). If any extension triggers an organization lookup during its initialization (a common scenario for resolving context), the resulting instrumentation data will be lost because engine.GetAnalytics() will return nil at that point. func newInstrumentationRecorder(engine workflow.Engine) instrumentationRecorder { r := instrumentationRecorder{} if a := engine.GetAnalytics(); a != nil { r.ic = a.GetInstrumentation() } return r }

📚 Repository Context Analyzed This review considered 10 relevant code sections from 7 files (average relevance: 0.92) pkg/analytics/analytics.go (2 segments, lines 1-245) pkg/workflow/analyticsWrapper.go (lines 1-100) pkg/analytics/instrumentation_collector.go (lines 1-288) pkg/workflow/enginewrapper.go (lines 1-125) pkg/local_workflows/report_analytics_workflow.go (lines 1-175) pkg/app/app.go (2 segments, lines 1-222) pkg/workflow/engineimpl.go (2 segments, lines 1-280)

[snyk-pr-review-bot]

PR Reviewer Guide 🔍

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Loss of Organization Context 🟠 [major] The removal of a.SetOrg() in initAnalytics() prevents the organization identifier from being associated with the Analytics object during initialization. According to the GetRequest() implementation in pkg/analytics/analytics.go (see cross-file context), the org field is used to append an org query parameter to the analytics API request URL. Without this being set, all telemetry sent by the engine will lose its organization association, likely breaking server-side filtering or attribution. func (e *EngineImpl) initAnalytics() analytics.Analytics { a := analytics.New() a.SetIntegration(e.config.GetString(configuration.INTEGRATION_NAME), e.config.GetString(configuration.INTEGRATION_VERSION)) a.SetApiUrl(e.config.GetString(configuration.API_URL)) a.SetClient(func() *http.Client { return e.networkAccess.GetHttpClient() }) Telemetry Silent Failure 🟡 [minor] The newInstrumentationRecorder function retrieves analytics via engine.GetAnalytics(). However, in the CreateAppEngineWithOptions flow, initConfiguration (which invokes these default functions) is called BEFORE engine.Init(). Since engine.analytics is only initialized during Init() (in engineimpl.go), the GetAnalytics() call will return nil during the initial configuration phase. This causes the new instrumentation to silently skip recording any data during the very lookup process it was designed to monitor. func newInstrumentationRecorder(engine workflow.Engine) instrumentationRecorder { r := instrumentationRecorder{} if a := engine.GetAnalytics(); a != nil { r.ic = a.GetInstrumentation() } return r }

📚 Repository Context Analyzed This review considered 10 relevant code sections from 7 files (average relevance: 0.92) pkg/analytics/analytics.go (2 segments, lines 1-245) pkg/workflow/analyticsWrapper.go (lines 1-100) pkg/analytics/instrumentation_collector.go (lines 1-288) pkg/workflow/enginewrapper.go (lines 1-125) pkg/local_workflows/report_analytics_workflow.go (lines 1-175) pkg/app/app.go (2 segments, lines 1-221) pkg/workflow/engineimpl.go (2 segments, lines 1-280)

[robertolopezlopez]

LGTM

[j-luong]

context: key tries to define the kind of data we're assessing in OTel standard dot . notation.
The idea is to break it down as follows: <project>.<package|scope>.<method>.<property>.<detail>.

[snyk-pr-review-bot]

PR Reviewer Guide 🔍

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Ambiguous Telemetry 🟡 [minor] In defaultFuncOrganization, when a UUID is provided directly, orgLookupProvidedID is recorded. However, unlike the slug lookup path which validates the slug against the API, the ID path returns the value immediately without verification. This results in telemetry that treats any syntactically valid UUID as a successful lookup, even if that ID does not exist or the user lacks access, making it indistinguishable from valid lookups in instrumentation data. instrumentation.addExtension(orgLookupSourceKey, orgLookupProvidedID) return orgId, nil

📚 Repository Context Analyzed This review considered 10 relevant code sections from 6 files (average relevance: 0.84) pkg/analytics/analytics.go (2 segments, lines 1-245) pkg/workflow/analyticsWrapper.go (lines 1-100) pkg/analytics/instrumentation_collector.go (lines 1-288) pkg/workflow/enginewrapper.go (lines 1-125) pkg/app/app.go (3 segments, lines 1-189) pkg/workflow/engineimpl.go (2 segments, lines 1-280)