Allow DAs to customize timeoutsand apps to control logging #1497
Conversation
|
|
|
@robmoffat I think this and the new tests are working well now. N.b. I needed to test from the fdc3-get-agent package (although the timeouts are in the fdc3-agent-proxy package) as the custom timeouts are set by the DA in WCP3Handshake responses (which are the province of |
|
I had to merge the debug logging PR into this one to figure out the tests (needed debug output from the proxy messages). After doing so I realized that every call to broadcast on the current user channel is preceeded by a call to get the current user channel... I think this should be refined for performance reasons and the current user channel tracked in the channel support - there is, after all, a message sent when the channel is changed... Interested in your thoughts on that @robmoffat. |
…r than the app timing it - Desktop Agent should report IntentDelvieryFailed itself)
|
|
|
Just looking at code coverage, I see cleanup() being called on all the MessageHandelrs except the HeartbeatHandler - any idea why? |
| type ColorFn = (aString: string) => string; | ||
|
|
||
| export abstract class AbstractFDC3Logger { | ||
| private static enableDebug: boolean = false; |
There was a problem hiding this comment.
It doesn't really make sense to me that we have two booleans for this behaviour. I think that might be what you're referring to in your slack comment. I think that suggestion is a lot better.
There was a problem hiding this comment.
Could you take a closer look at that suggestion - the problem with it is that if you want to be able to disable heartbeat logging separately from other debug logging (and you need to as the heartbeats really get in the way) then we need to move all other logs up to the log level so that debug is just heartbeats so you can turn just them off. Then we'd also want to set the default level above log so that we're not logging every message exchange by default.
If thats ok with you I can action it - but wanted to make sure you understand the implications before I rework it all.
There was a problem hiding this comment.
Yeah I must admit having re-read it I'm slightly confused about what it all means. Wouldn't it be better to have:
{
connection: "debug" | "log" | "warn" | "error" | "none" // I guess some of these levels aren't used.
proxy: "debug" | "log" | "warn" | "error" | "none"
heartbeat: "debug" | "log" | "warn" | "error" | "none"
}
... so you can choose the level of logging enabled for each part?
Normally, this would be achieved with hierarchical loggers but this is probably overkill here.
There was a problem hiding this comment.
It doesn't make sense to have a level set for heartbeat as its just one log message (that gets printed all. the. time.)...
What I'm suggesting is:
{
connection: "debug" | "log" | "warn" | "error" | "none" //default: "log", all levels are used!
proxy: "debug" | "log" | "warn" | "error" | "none" //default: "warn", heartbeats at debug, message exchanges at log-level
}
How does that sit with you?
| ) { | ||
| this.heartbeat = heartbeat; | ||
| this.intents = intents; | ||
| this.channels = channels; | ||
| this.apps = apps; | ||
| this.connectables = connectables; | ||
| Logger.enableHeartbeatLogs(logging.heartbeat); | ||
| Logger.enableDebugLogs(logging.debug); |
There was a problem hiding this comment.
might be tidier as Logger.configure(logging)
There was a problem hiding this comment.
Will await a decision on the comment above (using a log level setting instead and moving all messages except heartbeats to above the debug level) first as that would achieve the same.
| connectionAttemptUuid: this.connectionAttemptUuid, | ||
| handshake: data, | ||
| messagePort: event.ports[0], | ||
| options: this.options, | ||
| actualUrl: globalThis.window.location.href, | ||
| agentType: this.agentType, | ||
| agentUrl: this.agentUrl ?? undefined, | ||
| }); | ||
| defaultTimeout: data.payload.defaultTimeout ?? DEFAULT_MESSAGE_EXCHANGE_TIMEOUT_MS, |
There was a problem hiding this comment.
For clarity, it would be better to call this messageExchangeTimeout I think
| @@ -5,6 +5,7 @@ import { DesktopAgentSelection, Loader } from './Loader'; | |||
| import { HelloHandler } from './HelloHandler'; | |||
| import { IdentityValidationHandler } from './IdentityValidationHandler'; | |||
| import { Logger } from '../util/Logger'; | |||
| import { DesktopAgentProxyLogSettings } from '@finos/fdc3-agent-proxy/src/DesktopAgentProxy'; | |||
There was a problem hiding this comment.
Why do we have two different types here?
export type GetAgentLogSettings = {
connection: boolean;
connectionDebug: boolean;
proxyDebug: boolean;
heartbeat: boolean;
}
and
export type DesktopAgentProxyLogSettings = {
heartbeat: boolean;
debug: boolean;
};
It seems like at least two of these overlap, so shouldn't there be type addition happening at the least, if not use of the same type?
There was a problem hiding this comment.
The first is the logging argument to GetAgent which cover both GetAgent and AgentProxy log settings, the second is just that which applies to the proxy (and hence has half the properties). Hence, can't/shouldn't use the same type although they could be unioned, except that the first is in fdc3-standard, the second in downstream implementation projects. All-in-all, meh.
Will decide what to do about it after theres a decision on the log setting.
| @@ -0,0 +1,6 @@ | |||
| /** Default timeout used for all message exchanges with Desktop Agents, except | |||
| * those that involve the launch of an application. */ | |||
| export const DEFAULT_MESSAGE_EXCHANGE_TIMEOUT_MS = 10000; | |||
There was a problem hiding this comment.
curious that this is here instead of in the getAgent.ts file, since that also has a DEFAULT_TIMEOUT_MS in it.
There was a problem hiding this comment.
Different timeouts - message exchange vs. DA discovery. Changing the name to MESSAGE_EXCHANGE_TIMEOUT_MS (as you suggest) will resolve.
There was a problem hiding this comment.
ok, so we end up with all the different default timeouts in the Timeouts.ts file, right?
There was a problem hiding this comment.
I'll see what I can do. The original DEFAULT_TIMEOUT_MS for GetAgent is fdc3-standard/src/GetAgent.ts, while these are in fdc3-get-agent, as they're more implementation related. They should all be cosnilidated - I guess all into fdc3-standard/src/GetAgent.ts and drop Timeouts.ts?
| @@ -54,6 +54,18 @@ | |||
| "type": "boolean" | |||
| } | |||
| ] | |||
| }, | |||
| "defaultTimeout": { | |||
| "title": "Default Timeout", | |||
There was a problem hiding this comment.
yeah calling this this default timeout to override the default timeout... is pretty confusing
There was a problem hiding this comment.
Yeah lets change that to messageExchangeTimeout
The HelloHandler's cleanup function should be getting called but it has no body, just a TODO: The heartbeatHandler is only created if heartbeats are enabled: ...and you've got heartbeats disabled in most of the tests, see: There is one step that enables it: perhaps you are not shutting down in that one? Thats all outside the scope of this PR so I'll leave you to look at that. Found a number of other fish that need frying... |
kriswest
changed the title
|
/netlify |
|
@robmoffat not sure why netlify is ignoring this PR... |
I can't figure it out! @TheJuanAndOnly99 any ideas? |
…elete a redundant step
|
Closing to re-open (in case that wakes netlify up) |
|
|
1 similar comment
|
|
|
Hey @kriswest @robmoffat If I'm not mistaken Netlify will only generate previews for PRs made against the production branch |
|
@TheJuanAndOnly99 This PR got a preview and was targetting the same (non-main) branch: #1495 ...hmm its possible I raised that against main, then changed it... perhaps thats it. |
|
/netlify |
|
Switching to main as the base doesn't seem to get going on a build unfortunately. We might be in close and re-open territory. @robmoffat I'll make the changes to the logging args which should deal with all your comments then open as a new PR? |
|
|
Describe your change
Resolves two requested additions to getAgent/fdc3-for-web:
Adds the ability to control debug logging output from getAgent() and the DesktopAgentProxy via new arguments to getAgent(). The new arguments are documented in the types and documentation pages.
Allows DesktopAgents to set two custom timeouts, one for most FDC3 for Web message exchanges and a second for message exchanges that involve application launches. Also extends the default timeout for application launch message exchanges to 100 seconds and introduces standardized error messages for
ApiTimeout.Related Issue
resolves #1487
resolves #1488
Contributor License Agreement
Review Checklist
DesktopAgent,Channel,PrivateChannel,Listener,Bridging)?JSDoc comments on interfaces and types should be matched to the main documentation in /docs
Conformance test definitions should cover all required aspects of an FDC3 Desktop Agent implementation, which are usually marked with a MUST keyword, and optional features (SHOULD or MAY) where the format of those features is defined
The Web Connection protocol and Desktop Agent Communication Protocol schemas must be able to support all necessary aspects of the Desktop Agent API, while Bridging must support those aspects necessary for Desktop Agents to communicate with each other
npm run build) run and the results checked in?Generated code will be found at
/src/api/BrowserTypes.tsand/or/src/bridging/BridgingTypes.ts