-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Include umbraco-package.json manifests in telemetry data #16430
Conversation
Heyo 👋 |
@@ -155,7 +183,8 @@ public async Task<PagedModel<PackageDefinition>> GetCreatedPackagesAsync(int ski | |||
using ICoreScope scope = _coreScopeProvider.CreateCoreScope(autoComplete: true); | |||
PackageDefinition[] packages = _createdPackages.GetAll().WhereNotNull().ToArray(); | |||
var pagedModel = new PagedModel<PackageDefinition>(packages.Length, packages.Skip(skip).Take(take)); | |||
return await Task.FromResult(pagedModel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed we have quite a few occurrences of await Task.FromResult()
spread throughout the codebase, which seems wasteful, as it wraps the result and adds an additional await (with all the extra async state machine code that's generated by the compiler)...
I've avoided any breaking changes now and added comments to highlight some additional bugs/issues that are fixed in this PR. Would be great if we can ship this in a patch, instead of waiting for the next minor 😇 |
To help testing out the changes, you can put this composer/component in your project and check out the logged messages: using Umbraco.Cms.Core.Composing;
using Umbraco.Cms.Core.Telemetry;
internal sealed class LogPackageTelemetryComposer : IComposer
{
public void Compose(IUmbracoBuilder builder)
{
builder.AddComponent<LogPackageTelemetryComponent>();
}
private sealed class LogPackageTelemetryComponent : IComponent
{
private readonly ITelemetryService _telemetryService;
private readonly ILogger<LogPackageTelemetryComponent> _logger;
public LogPackageTelemetryComponent(ITelemetryService telemetryService, ILogger<LogPackageTelemetryComponent> logger)
{
_telemetryService = telemetryService;
_logger = logger;
}
public void Initialize()
{
var telemetryReportData = _telemetryService.GetTelemetryReportDataAsync().GetAwaiter().GetResult();
if (telemetryReportData?.Packages is not null)
{
foreach (var packageTelemetry in telemetryReportData.Packages)
{
_logger.LogInformation(
"Package telemetry for {PackageName} ({PackageId}) {PackageVersion}",
packageTelemetry.Name,
packageTelemetry.Id,
packageTelemetry.Version);
}
}
}
public void Terminate()
{ }
}
} On a side note: this does highlight the fact we don't have an |
Looks good, tests good 🚀 |
Prerequisites
Description
PR #15744 removed the legacy package manifest parser, but also removed the code from
PackagingService
that returned the combined the package migrations and manifests. This data is used in the telemetry service to report the installed packages (if the consent level of the installation is set to Basic or Detailed and the package hasn't opted-out), which is mainly used in the Marketplace to sort the packages by most active installs.Another important detail that was missing, is being able to specify the (NuGet) package ID in the manifest, so the Marketplace can do a 1:1 match when aggregating the data.
Besides re-introducing the package manifests as installed packages and adding the package ID, I've ensured the ID is also exposed in the Management API endpoints. E.g. the
umbraco/management/api/v1/manifest/manifest
endpoint now returns:Testing can be done by adding a
umbraco-package.json
file to e.g.wwwroot\App_Plugins\Test
:And then making sure this is included in
ReportSiteJob.RunJobAsync()
(update Delay to 0 and add a breakpoint to this method) and the package ID is exposed in the Management API (all Manifest endpoints).