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

Question: Application discovering new metrics #43

Closed
Kvargefar opened this issue Mar 2, 2023 · 9 comments
Closed

Question: Application discovering new metrics #43

Kvargefar opened this issue Mar 2, 2023 · 9 comments
Assignees
Labels
question Further information is requested

Comments

@Kvargefar
Copy link

Hi,

###Validation of incoming metrics
I've been working a little with the library for a while. I noticed that the application ignores metrics that do not already exist in KnownMetrics through DataReceived. I therefore wonder if the only way for the application to discover new metrics from a fresh or old node/device is through birth?
I will either have to ask for rebirth when I come online, or store old metrics in a file and use old known metrics in the application's constructor?

###DeviceStates and NodeStates
Moreover, the DeviceStates and NodeStates dicts store all nodes and devices discovered, with their belonging MetricState - which is great. But I noticed that the key for these dicts is a simple string representing the DeviceId or NodeId. It doesn't seem to store additional information about these. If you take a device as an example, it only stores the device identifier, but not the EoN identifier - which you need if you want to target this device with a command, for instance. Unless this info is possible to get somehow?
For my case, I want to store all metrics in a database. I might have to append the metric name to the device or node it originated from.

@Kvargefar Kvargefar reopened this Mar 10, 2023
@Kvargefar
Copy link
Author

Kvargefar commented Mar 10, 2023

It seems like this line of code makes the application ignore incoming data.
How can the application discover new nodes and new data, if it ignores DDATA and NDATA payloads? The application needs to be able to discover new nodes and devices, and get their name, to be able to ask for a rebirth.

this.KnownMetricsStorage.ValidateIncomingMetrics(metricsWithoutSequenceMetric);

@SeppPenner SeppPenner self-assigned this Mar 14, 2023
@SeppPenner SeppPenner added the question Further information is requested label Mar 14, 2023
@SeppPenner
Copy link
Owner

For the first question: According to 2.2 spec, this is by design, check out #6 (comment) as well. No idea how this is according to the 3.0 spec or if it's better specified in there. Need to check this.

For the second question: You are right. Seems to be a good idea to store the EON node identifier there, too. I need to rework the library according to 3.0 spec either way, so I hope to check all the open issues within this step.

@BoBiene
Copy link
Contributor

BoBiene commented Mar 21, 2023

You can provide your own metric storage and implement a custom logic to validate the metric in your client code.

@SeppPenner
Copy link
Owner

SeppPenner commented Apr 7, 2023

After reading through the specs, this is according to the 3.0 version as well: (Excerpt from page 21 which is PDF page 28):

The NBIRTH MUST include every metric the Edge Node will ever report on.
At a minimum each metric MUST include the metric name, datatype, and current value.
If Template instances will be published by this Edge Node or any devices, all Template definitions MUST be published in the NBIRTH.

So, to be clear: The library works according to the spec and I'm not willing to change this.

Storing the EON node identifier in the dictionary key is ok, I will implement that.

EDIT: Maybe the requested feature to trigger a device / node rebirth is an option (I need to check whether this is allowed by the spec or not).

@EricRosenfeld1
Copy link

Correct me if I'm wrong, but don't the specs indicate that the NBIRTH must include all the metrics the node will report? Right now the SparkplugApplication needs to already know these metrics.

The call to this.KnownMetricsStorage.ValidateIncomingMetrics(metricsWithoutSequenceMetric); needs to exclude NBIRTH topics.

@jeff-pf
Copy link

jeff-pf commented Nov 27, 2023

Node birth message retain flag must be set to false - only "Sparkplug Aware" (section 10.1.4 of v3.0.0 spec) brokers retain those messages. There needs to be a method to send rebirth message to nodes that are already online and send data after the application starts for brokers that are not "Sparkplug Aware". Instead of doing this automatically - which does not give the application any control - have the event of data published without a birth message and a method to call to send a rebirth. If the application is interested in that node it can send a rebirth.

@jeff-pf
Copy link

jeff-pf commented Nov 28, 2023

@EricRosenfeld1

This call does exclude NBIRTH messages because metricsWithoutSequenceMetric is actually messages without bdSeq metric - which is the birth / death sequence number.

    var metricsWithoutSequenceMetric = payload.Metrics.Where(m => m.Name != Constants.SessionNumberMetricName);   

@SeppPenner
Copy link
Owner

Node birth message retain flag must be set to false - only "Sparkplug Aware" (section 10.1.4 of v3.0.0 spec) brokers retain those messages. There needs to be a method to send rebirth message to nodes that are already online and send data after the application starts for brokers that are not "Sparkplug Aware". Instead of doing this automatically - which does not give the application any control - have the event of data published without a birth message and a method to call to send a rebirth. If the application is interested in that node it can send a rebirth.

This was already done sometime before.

@SeppPenner
Copy link
Owner

Moreover, the DeviceStates and NodeStates dicts store all nodes and devices discovered, with their belonging MetricState - which is great. But I noticed that the key for these dicts is a simple string representing the DeviceId or NodeId. It doesn't seem to store additional information about these. If you take a device as an example, it only stores the device identifier, but not the EoN identifier

I added this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants