Skip to content
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

[Telemetry API] Prevent Subscriptions with different options from overwriting each other #7930

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open
145 changes: 145 additions & 0 deletions e2e/tests/functional/plugins/displayLayout/displayLayout.e2e.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -507,8 +507,153 @@ test.describe('Display Layout', () => {
// In real time mode, we don't fetch annotations at all
await expect.poll(() => networkRequests, { timeout: 10000 }).toHaveLength(0);
});

test('Same objects with different request options have unique subscriptions', async ({
page
}) => {
// Expand My Items
await page.getByLabel('Expand My Items folder').click();

// Create a Display Layout
const displayLayout = await createDomainObjectWithDefaults(page, {
type: 'Display Layout',
name: 'Test Display'
});

// Create a State Generator, set to higher frequency updates
const stateGenerator = await createDomainObjectWithDefaults(page, {
type: 'State Generator',
name: 'State Generator'
});
const stateGeneratorTreeItem = page.getByRole('treeitem', {
name: stateGenerator.name
});
await stateGeneratorTreeItem.click({ button: 'right' });
await page.getByLabel('Edit Properties...').click();
await page.getByLabel('State Duration (seconds)', { exact: true }).fill('0.1');
await page.getByLabel('Save').click();

// Create a Table for filtering ON values
const tableFilterOnValue = await createDomainObjectWithDefaults(page, {
type: 'Telemetry Table',
name: 'Table Filter On Value'
});
const tableFilterOnTreeItem = page.getByRole('treeitem', {
name: tableFilterOnValue.name
});

// Create a Table for filtering OFF values
const tableFilterOffValue = await createDomainObjectWithDefaults(page, {
type: 'Telemetry Table',
name: 'Table Filter Off Value'
});
const tableFilterOffTreeItem = page.getByRole('treeitem', {
name: tableFilterOffValue.name
});

// Navigate to ON filtering table and add state generator and setup filters
await page.goto(tableFilterOnValue.url);
await stateGeneratorTreeItem.dragTo(page.getByLabel('Object View'));
await selectFilterOption(page, '1');
await page.getByLabel('Save').click();
await page.getByRole('listitem', { name: 'Save and Finish Editing' }).click();

// Navigate to OFF filtering table and add state generator and setup filters
await page.goto(tableFilterOffValue.url);
await stateGeneratorTreeItem.dragTo(page.getByLabel('Object View'));
await selectFilterOption(page, '0');
await page.getByLabel('Save').click();
await page.getByRole('listitem', { name: 'Save and Finish Editing' }).click();

// Navigate to the display layout and edit it
await page.goto(displayLayout.url);

// Add the tables to the display layout
await page.getByLabel('Edit Object').click();
await tableFilterOffTreeItem.dragTo(page.getByLabel('Layout Grid'), {
targetPosition: { x: 10, y: 300 }
});
await page.locator('.c-frame-edit > div:nth-child(4)').dragTo(page.getByLabel('Layout Grid'), {
targetPosition: { x: 400, y: 500 },
// eslint-disable-next-line playwright/no-force-option
force: true
});
await tableFilterOnTreeItem.dragTo(page.getByLabel('Layout Grid'), {
targetPosition: { x: 10, y: 100 }
});
await page.locator('.c-frame-edit > div:nth-child(4)').dragTo(page.getByLabel('Layout Grid'), {
targetPosition: { x: 400, y: 300 },
// eslint-disable-next-line playwright/no-force-option
force: true
});
await page.getByLabel('Save').click();
await page.getByRole('listitem', { name: 'Save and Finish Editing' }).click();

// Get the tables so we can verify filtering is working as expected
const tableFilterOn = page.getByLabel(`${tableFilterOnValue.name} Frame`, {
exact: true
});
const tableFilterOff = page.getByLabel(`${tableFilterOffValue.name} Frame`, {
exact: true
});

// Verify filtering is working correctly

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// Create a promise that resolves when we've seen enough new rows added
const rowsMutationPromise = page.evaluate(() => {
return new Promise((resolve) => {
const targetTable = document.querySelector(
'table[aria-label="Table Filter Off Value table content"]'
);
const config = { childList: true, subtree: true };
let changeCount = 0;
const requiredChanges = 20; // Number of changes to wait for

const observer = new MutationObserver((mutations) => {
mutations.forEach((mutation) => {
if (mutation.type === 'childList') {
// Count added nodes
changeCount += mutation.addedNodes.length;
}
});

// Check if the required number of changes has been met
if (changeCount >= requiredChanges) {
observer.disconnect(); // Disconnect observer after the required changes
resolve();
}
});

observer.observe(targetTable, config);
});
});

await rowsMutationPromise;

// Check ON table doesn't have any OFF values
await expect(tableFilterOn.locator('td[title="OFF"]')).toHaveCount(0);
const onCount = await tableFilterOn.locator('td[title="ON"]').count();
await expect(onCount).toBeGreaterThan(0);

// Check OFF table doesn't have any ON values
await expect(tableFilterOff.locator('td[title="ON"]')).toHaveCount(0);
const offCount = await tableFilterOff.locator('td[title="OFF"]').count();
await expect(offCount).toBeGreaterThan(0);
});
});

async function selectFilterOption(page, filterOption) {
await page.getByRole('tab', { name: 'Filters' }).click();
await page
.getByLabel('Inspector Views')
.locator('li')
.filter({ hasText: 'State Generator' })
.locator('span')
.click();
await page.getByRole('switch').click();
await page.selectOption('select[name="setSelectionThreshold"]', filterOption);
}

async function addAndRemoveDrawingObjectAndAssert(page, layoutObject, DISPLAY_LAYOUT_NAME) {
await expect(page.getByLabel(layoutObject, { exact: true })).toHaveCount(0);
await addLayoutObject(page, DISPLAY_LAYOUT_NAME, layoutObject);
Expand Down
10 changes: 10 additions & 0 deletions example/generator/GeneratorMetadataProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,16 @@ const METADATA_BY_TYPE = {
string: 'ON'
}
],
filters: [
{
singleSelectionThreshold: true,
comparator: 'equals',
possibleValues: [
{ label: 'OFF', value: 0 },
{ label: 'ON', value: 1 }
]
}
],
hints: {
range: 1
}
Expand Down
28 changes: 23 additions & 5 deletions example/generator/StateGeneratorProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,16 @@ StateGeneratorProvider.prototype.supportsSubscribe = function (domainObject) {
return domainObject.type === 'example.state-generator';
};

StateGeneratorProvider.prototype.subscribe = function (domainObject, callback) {
StateGeneratorProvider.prototype.subscribe = function (domainObject, callback, options) {
var duration = domainObject.telemetry.duration * 1000;

var interval = setInterval(function () {
var interval = setInterval(() => {
var now = Date.now();
var datum = pointForTimestamp(now, duration, domainObject.name);
datum.value = String(datum.value);
callback(datum);
if (!this.shouldBeFiltered(datum, options)) {
datum.value = String(datum.value);
callback(datum);
}
}, duration);

return function () {
Expand All @@ -63,9 +65,25 @@ StateGeneratorProvider.prototype.request = function (domainObject, options) {

var data = [];
while (start <= end && data.length < 5000) {
data.push(pointForTimestamp(start, duration, domainObject.name));
const point = pointForTimestamp(start, duration, domainObject.name);

if (!this.shouldBeFiltered(point, options)) {
data.push(point);
}
start += duration;
}

return Promise.resolve(data);
};

StateGeneratorProvider.prototype.shouldBeFiltered = function (point, options) {
const valueToFilter = options?.filters?.state?.equals?.[0];

if (!valueToFilter) {
return false;
}

const { value } = point;

return value !== Number(valueToFilter);
};
103 changes: 99 additions & 4 deletions src/api/telemetry/TelemetryAPI.js
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,103 @@ export default class TelemetryAPI {
return options;
}

/**
* Sanitizes objects for consistent serialization by:
* 1. Converting non-plain objects (class instances) and functions to string markers (e.g. "@keyName")
* 2. Sorting object keys alphabetically to ensure consistent ordering
* 3. Recursively processing nested objects and arrays
*
* This is primarily used to generate consistent hash values for telemetry subscription options,
* ensuring that equivalent options objects produce the same hash regardless of property order
* or the presence of non-serializable values. While this would normally be a private method,
* it is exposed for use in JSON.stringify.
*
* @param {string} key The current property key being processed
* @param {Object|Array|*} value The value to sanitize
* @returns {Object|Array|string|*} The sanitized value:
* - Plain objects: A new object with sorted keys and sanitized values
* - Arrays: A new array with sanitized elements
* - Functions/Class instances: String in format "@keyName"
* - Primitives: Returned as-is
*/
sanitizeForSerialization(key, value) {
// Handle null and primitives directly
if (value === null || typeof value !== 'object') {
return value;
}

// Handle arrays
if (Array.isArray(value)) {
return value.map((item, index) => this.sanitizeForSerialization(String(index), item));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just return the array itself, the replacer function (sanitizeForSerialization) will automatically be called for each member. see docs - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#the_replacer_parameter

}

// Replace functions and non-plain objects with their key names
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return undefined to leave the property out of its stringified representation entirely, per the docs - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#the_replacer_parameter

if (typeof value === 'function' || Object.getPrototypeOf(value) !== Object.prototype) {
return `@${key}`;
}

// Handle plain objects
const sortedObject = {};
const keys = Object.keys(value).sort();
for (const objectKey of keys) {
const itemValue = value[objectKey];
const sanitizedValue = this.sanitizeForSerialization(objectKey, itemValue);
sortedObject[objectKey] = sanitizedValue;
}

return sortedObject;
}

/**
* Generates a numeric hash value for an options object. The hash is consistent
* for equivalent option objects regardless of property order.
*
* This is used to create compact, unique cache keys for telemetry subscriptions with
* different options configurations. The hash function ensures that identical options
* objects will always generate the same hash value, while different options objects
* (even with small differences) will generate different hash values.
*
* @private
* @param {Object} options The options object to hash
* @returns {number} A positive integer hash of the options object
*/
#hashOptions(options) {
const sanitizedOptionsString = JSON.stringify(
options,
this.sanitizeForSerialization.bind(this)
);

let hash = 0;
const prime = 31;
const modulus = 1e9 + 9; // Large prime number

for (let i = 0; i < sanitizedOptionsString.length; i++) {
const char = sanitizedOptionsString.charCodeAt(i);
// Calculate new hash value while keeping numbers manageable
hash = Math.floor((hash * prime + char) % modulus);
}

return Math.abs(hash);
}

/**
* Generates a unique cache key for a telemetry subscription based on the
* domain object identifier and options (which includes strategy).
*
* Uses a hash of the options object to create compact cache keys while still
* ensuring unique keys for different subscription configurations.
*
* @private
* @param {import('openmct').DomainObject} domainObject The domain object being subscribed to
* @param {Object} options The subscription options object (including strategy)
* @returns {string} A unique key string for caching the subscription
*/
#getSubscriptionCacheKey(domainObject, options) {
const keyString = makeKeyString(domainObject.identifier);

return `${keyString}:${this.#hashOptions(options)}`;
}

/**
* Register a request interceptor that transforms a request via module:openmct.TelemetryAPI.request
* The request will be modified when it is received and will be returned in it's modified state
Expand Down Expand Up @@ -418,16 +515,14 @@ export default class TelemetryAPI {
this.#subscribeCache = {};
}

const keyString = makeKeyString(domainObject.identifier);
const supportedStrategy = supportsBatching ? requestedStrategy : SUBSCRIBE_STRATEGY.LATEST;
// Override the requested strategy with the strategy supported by the provider
const optionsWithSupportedStrategy = {
...options,
strategy: supportedStrategy
};
// If batching is supported, we need to cache a subscription for each strategy -
// latest and batched.
const cacheKey = `${keyString}:${supportedStrategy}`;

const cacheKey = this.#getSubscriptionCacheKey(domainObject, optionsWithSupportedStrategy);
let subscriber = this.#subscribeCache[cacheKey];

if (!subscriber) {
Expand Down
6 changes: 3 additions & 3 deletions src/plugins/filters/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ To define a filter, you'll need to add a new `filter` property to the domain obj
singleSelectionThreshold: true,
comparator: 'equals',
possibleValues: [
{ name: 'Apple', value: 'apple' },
{ name: 'Banana', value: 'banana' },
{ name: 'Orange', value: 'orange' }
{ label: 'Apple', value: 'apple' },
{ label: 'Banana', value: 'banana' },
{ label: 'Orange', value: 'orange' }
]
}]
}
Expand Down
Loading