Adds visible Content Manager progress during realm sync/revert operations and improves content download resilience for large syncs.#4604
Conversation
Move the ambient microservice log-level AsyncLocal into beamable.tooling.common so AdminRoutes and runtime logging share the same context without introducing a runtime dependency. Scope OpenAPI docs generation to temporarily raise the log threshold to Warning and restore the previous level afterward. Preserve the existing MicroserviceBootstrapper.ContextLogLevel API as an obsolete forwarder for compatibility.
- SSL error fix during content sync
- updated changelog
|
Lightbeam link |
allister-beamable
left a comment
allister-beamable
left a comment
There was a problem hiding this comment.
Let's be very careful about separation of concerns here. I see four mostly-unrelated categories of changes, three beneficial and one probably by mistake:
- reuse
HttpClients for content downloads and add configurable parallelism - add a rich progress bar to content downloading in Unity Editor
- suppress overly verbose logs
- remove OTel-related setup in
BeamableMicroservice.cs
The first two seem fine to bundle together as a single feature addition. The third and fourth are a little more dubious. I think the third has value but might not belong here, and the fourth is either a mistake or a diff-unfriendly way of making changes. I would like to see some discussion of the latter two before this lands on main.
| // Suppress info-level logs during OpenAPI doc generation to avoid noise | ||
| //Wrapping with try/finally to ensure log level is restored even if an exception occurs | ||
| var previousLogLevel = MicroserviceLogLevelContext.CurrentLogLevel.Value; | ||
| MicroserviceLogLevelContext.CurrentLogLevel.Value = LogLevel.Warning; |
There was a problem hiding this comment.
This seems to introduce stuff that is not directly germane to the content downloading part. Are we okay with that?
I think my vote is "yes, this is an okay digression" as the value of this addition outweighs the bad smell (violation of "separation of concerns"), but I think it is worth making the decision consciously.
|
|
||
| MicroserviceRequestContext ctx = null; | ||
| using (var requestActivity = _activityProvider.Create(Otel.TRACE_CONSTRUCT_CTX, importance: TelemetryImportance.VERBOSE)) | ||
| { | ||
| if (!document.TryBuildRequestContext(InstanceArgs, out ctx)) | ||
| { | ||
| requestActivity.SetStatus(ActivityStatusCode.Error); | ||
| Log.Debug("WS Message contains no data. Cannot handle. Skipping message."); | ||
| return; | ||
| } | ||
| requestActivity.SetStatus(ActivityStatusCode.Ok); | ||
| } | ||
|
|
||
| BeamActivity parentActivity = BeamActivity.Noop; | ||
| if (_socketRequesterContext.TryGetListener(ctx.Id, out var existingListener)) | ||
| { | ||
| parentActivity = existingListener.Activity; | ||
| } | ||
|
|
||
|
|
||
| using var activity = _activityProvider.Create(Otel.TRACE_WS, parentActivity); | ||
| MicroserviceRequester.ContextActivity.Value = activity; | ||
| ctx.ActivityContext = activity; | ||
|
|
||
|
|
||
| var extraOtelTags = _telemetryProviders.CreateRequestAttributes(InstanceArgs, ctx, ConnectionId); | ||
| activity.SetTags(extraOtelTags); | ||
|
|
||
|
|
||
| // First get the Global Realm Config Log Level and apply it by running UpdateLogLevel | ||
| var configService = InstanceArgs.ServiceScope.GetService<IRealmConfigService>(); | ||
| if (ctx.Path?.StartsWith(_adminPrefix) ?? false) | ||
| { | ||
| // when the path starts with admin, use warning. | ||
| MicroserviceBootstrapper.ContextLogLevel.Value = LogLevel.Warning; | ||
| } | ||
| else | ||
| { | ||
| // otherwise, allow default behaviour. | ||
| configService.UpdateLogLevel(); | ||
| } | ||
|
|
There was a problem hiding this comment.
This removal looks wrong. Did your branch get cross-contaminated with another branch that is dealing with OTel stuff @mhijaziB ?
DiasAtBeamable
left a comment
DiasAtBeamable
left a comment
There was a problem hiding this comment.
The Editor and Content changes seems fine for me. I just added a few comments to check a few things. Also, I think that it may have some files commited by mistake as they are not related to the content manager change.
| /// <remarks> | ||
| /// Returning the default int omits the CLI argument, allowing the CLI default to apply. A configured value of 0 maps to -1 so the generated command includes an explicit unbounded setting. | ||
| /// </remarks> | ||
| private int GetEditorSyncDownloadMaxParallelCountCliOption() |
There was a problem hiding this comment.
Why do you need to set it to -1? The CLI can accept 0 the same way it would handle -1. Also, you can use _contentConfiguration.EditorSyncDownloadMaxParallelCount.GetOrElse(0);
There was a problem hiding this comment.
Also, the CLI default is 64. Shouldn't we use that value as the default for Unity as well? What are the benefits of having Unity use unbounded by default?
| var docs = new ServiceDocGenerator(); | ||
| var ctx = GlobalProvider.GetService<StartupContext>(); | ||
| var doc = docs.Generate(ctx, GlobalProvider); | ||
| // Suppress info-level logs during OpenAPI doc generation to avoid noise |
There was a problem hiding this comment.
I don't think this is related to the Content Manager changes. Are you sure about adding this change to this PR?
| DEFAULT, STRUCTURED, UNSTRUCTURED, FILE, STRUCTURED_AND_FILE | ||
| } | ||
|
|
||
| public enum AdminRouteLogPolicy |
There was a problem hiding this comment.
Same here, seems unrelated to the ContentManager changes.
| { | ||
| throw new NotImplementedException(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Same here, seems unrelated to the ContentManager changes.
| { | ||
| public static class MicroserviceLogLevelContext | ||
| { | ||
| public static readonly AsyncLocal<LogLevel> CurrentLogLevel = new(); |
There was a problem hiding this comment.
Same here, seems unrelated to the ContentManager changes.
| { | ||
| LogUtil.TryParseSystemLogLevel(configuredArgs.OapiGenLogLevel, out var defaultLevel, LogLevel.Information); | ||
| MicroserviceBootstrapper.ContextLogLevel.Value = defaultLevel; | ||
| MicroserviceLogLevelContext.CurrentLogLevel.Value = defaultLevel; |
There was a problem hiding this comment.
Same here, seems unrelated to the ContentManager changes.
| @@ -0,0 +1,226 @@ | |||
| using Beamable.Api.Autogenerated.Models; | |||
There was a problem hiding this comment.
Same here, seems unrelated to the ContentManager changes.
| } | ||
| } | ||
|
|
||
| public class RecordingTelemetryAttributeProvider : ITelemetryAttributeProvider |
There was a problem hiding this comment.
Same here, seems unrelated to the ContentManager changes.
| { | ||
| if (exception is RequesterException requesterException) | ||
| { | ||
| return requesterException.Status is 408 or 429 || requesterException.Status >= 500 && requesterException.Status < 600; |
There was a problem hiding this comment.
Could you add comments here on why we consider those requester status code errors transient?
| { | ||
| genBeamCommandArgs.Add(("--target=" + this.target)); | ||
| } | ||
| // If the downloadMaxParallelCount value was not default, then add it to the list of args. |
There was a problem hiding this comment.
Was this autogenerated?
|
I have converted this to a draft-PR. Git has pushed changes were not supposed to appear here! I was wondering about the OTEL talk here! and now i know why! |
3 participants
Ticket
Fixes #4605
Hubspot ticket
Brief Description
Details
Added an in-window Content Manager progress display for sync/revert flows.
Shows operation title, processed item count, total item count, progress bar fill, and current content item name.
Keeps sync progress inside the Content Manager instead of relying on Unity’s modal progress UI.
Disables revert/action buttons while a sync operation is active to avoid overlapping operations.
Uses delayed window-state transitions to avoid IMGUI layout mismatches during repaint.
Improved CLI content download behavior during content sync.
Added a dedicated content-file download path in ContentService.
Uses a shared HttpClient for content file downloads instead of routing every CDN file through the generic verbose CLI requester.
Adds transient retry handling with jittered backoff for socket resets, timeouts, and retryable HTTP responses.
Removes per-file verbose “Downloading content with id...” sync log noise from the Unity CLI Debugger.
Keeps large syncs faster and cleaner while avoiding the SSL/socket reset failures seen during high-content-count tests.
Added sync download concurrency control.
New CLI flag: --download-max-parallel-count
New Unity content configuration field: EditorSyncDownloadMaxParallelCount
The editor passes this value into content sync when configured.
CLI default is 64.
Unity unset means the CLI default is used.
0 means unbounded parallelism.
Tests:
Checklist
Add those to list or remove the list below altogether:
Notes
When you are merging a feature branch into
main, please squash merge and make sure the final commit contains any relevent JIRA ticket number. If you are merging frommaintostaging, orstagingtoproduction, please use a regular merge commit.Does this introduce tech-debt? If so, have you added an entry to the Tech-debt document?