Analytics activities

[VeronikaSolovei9]

reportAnalytics integration: call before invoking each analytics adapter: /auction, /amp, /video, /event

[SyntaxNode]

Why are you combining the ComponentName and ComponentType fields here? They are handled separately, so this code just needs to parse ComponentName. Right?

[VeronikaSolovei9]

It looks like we need to incorporate componentType here too.
I added a comment with the description and example of what will happen if there are multiple component names and types.

    // condition can contain more than one component type and component name may have more than one names
// in this case scopes should have all combinations of names and types
// for instance for the config like this
//     "condition": {
//          "componentName": ["enabledAnalytics", "filelogger", "bidder.appnexus"],
//           "componentType":["analytics", "rtd"]
//     }
// result scopes should be:
// analytics.enabledAnalytics
// analytics.filelogger
// rtd.enabledAnalytics
// rtd.filelogger
// bidder.appnexus

I found that before this fix in case if we have a config like this:

"condition": {
        "componentName": [ "filelogger"],
         "componentType":["analytics"]
    }

result component object has componentType="bidder".

Should we rename this function to conditionToRuleComponent ?

[SyntaxNode]

result component object has componentType="bidder".

Right. That's the desired behavior. There is no guarantee of name uniqueness between adapters, analytics, and modules. The only way we know that filelogger is meant to be an analytics type is for that to be specified. Since adapters are most common, we made that the default for convience.

"componentName": [ "filelogger"] should be interpreted as bidder.filelogger.

in this case scopes should have all combinations of names and types

That's not my expectation and I don't believe that's in the spec.

[VeronikaSolovei9]

This is so confusing. My suggestion may not be right, it's easy to revert it back.
If I refer to the issue #2591 it says:

The following attributes may be seen:

componentName - this is the biddercode, the analytics adapter code, or the module code.
componentType - this is "bidder", "analytics", or for modules: if the module tag contains "rtd", then type is "rtd", otherwise type is "general".

In examples below it shows:

 rules: [{
          condition: {
            componentType: ["bidder", "analytics"]
          },
          allow: false
        }

This gave me the idea that config name can only have name and config type can only have a type and we will need to combine them together.

[SyntaxNode]

Interesting. Reviewing the history of issue #2591, it seems the dot-notation was removed the first day of the focus group. I didn't remember that change. It does offer a lot of simplification with the assumption that name conflicts between analytics, adapters, and modules are rare.

In this case, let's keep the target as a Component, but change the componentName constraint to simply an array of strings. We can delete the complex Component comparison logic. The new matching algorithm for componentName ignores the type and the componentType ignores the name. We don't need to compute the matrix of all possibilities. These comparisons should be case insensitive (I think they are already).

[SyntaxNode]

That would be a lot of tangental work to include in this PR. I put up #3081 for the change.

[VeronikaSolovei9]

I also tried to move analytics.config content one level up to analytics and got a circular dependency issue.

 imports github.com/prebid/prebid-server/router
 imports github.com/prebid/prebid-server/analytics
 imports github.com/prebid/prebid-server/analytics/filesystem
 imports github.com/prebid/prebid-server/analytics: import cycle not allowed

Do I still need to move it?

[hhhjort]

Just a nit pick, but I might suggest using a name like analytics rather than just config2 to better document what sort of package config2 represents.

[VeronikaSolovei9]

Renamed to analyticsConf, analytics are already imported in this file.

[hhhjort]

Just a naming nitpick to consider, but otherwise looking good now.

[VeronikaSolovei9]

Moved Runner to it's own go file to the root of analytics package.
I will follow your name suggestions for the "analytics.config" package. Maybe "analytics.composite"?

[SyntaxNode]

Moved Runner to it's own go file to the root of analytics package. I will follow your name suggestions for the "analytics.config" package. Maybe "analytics.composite"?

For adapters, we have exchange.BuildAdapters. For modules, we have modules.Build. Perhaps analytics.build as the package name?

[hhhjort]

LGTM

[VeronikaSolovei9]

Merged with the main branch. Please re-review.