eclipse-tracecompass / org.eclipse.tracecompass

Eclipse Trace Compass
https://eclipse.dev/tracecompass/
Eclipse Public License 2.0
13 stars 13 forks source link

tmf: add ITmfDataProviderConfigurator interface for custom data provider #154

Closed bhufmann closed 1 month ago

bhufmann commented 1 month ago

Add interface to implement to prepare a derived data provider with input parameter defined by ITmfConfigurationSourceType.

For that add interfaces ITmfDataProviderConfigurator for capabilities to create data providers that a data provider factory could implement.

Add getAdapter() interface to IDataProviderFactory to allow retrieving ITmfDataProviderConfigurator.

Example: ITmfDataProviderConfigurator configurator = factory.getAdapter(ITmfDataProviderConfigurator.class);

[Added] ITmfDataProviderConfigurator interface for custom data provider [Added] getAdapter() interface to IDataProviderFactory

For example, to get a ITmfDataProviderSource implementation do:

   ITmfDataProviderSource source = factory.getAdapter(IDataProviderSource.class);

Note that this PR includes changes of other PR (#141)

Signed-off-by: Bernd Hufmann bernd.hufmann@ericsson.com

abhinava-ericsson commented 1 month ago

I found the "*Source" classes to be some kind of a catch-all which doesn't convey much. I would suggest the following:

  1. Renaming (I)TmfDataProviderSource to (I)TmfDataProviderCustomizer
  2. Renaming (I)TmfConfigurationSourceType to (I)TmfConfigurationType
  3. Renaming (I)TmfConfigurationSource to (I)TmfConfigurationFactory
  4. Renaming TmfConfigurationSourceManager to TmfConfigurationManager

In case 2-4 have already been exposed in Trace Compass API, 1 alone would still be helpful.

bhufmann commented 1 month ago

I found the "*Source" classes to be some kind of a catch-all which doesn't convey much. I would suggest the following:

  1. Renaming (I)TmfDataProviderSource to (I)TmfDataProviderCustomizer
  2. Renaming (I)TmfConfigurationSourceType to (I)TmfConfigurationType
  3. Renaming (I)TmfConfigurationSource to (I)TmfConfigurationFactory
  4. Renaming TmfConfigurationSourceManager to TmfConfigurationManager

In case 2-4 have already been exposed in Trace Compass API, 1 alone would still be helpful.

Thanks for the suggestions.

As you already realized 2-4 are already exposed as APIs. They were introduced to manage (create/update/delete) global configurations, e.g. managing XML data-driven analysis defined in XML files. The TmfConfigurationSourceManager manages instances of TmfConfigurationSource defined as extensions to the corresponding extension point. Each TmfConfigurationSource has a type TmfConfigurationSourceType that describes the source. Each source is then managing instances of ITmfConfiguration. Hence the term TmfConfigurationManager as replacement to TmfConfigurationTypeManager would not fit, because each source is managing its configurationsm and hence are the manager of configurations. I chose the different name TmfConfigurationTypeManager to distinguish this different difference

My plan from the beginning was to standardize interfaces and implementation for all kinds of configurations of the Trace Compass backend, and reuse them for different use cases. . For the case 1, it's not API yet and we can change the name. I chose a name with source in the name to be consistent with other configuration implementations (2-4). I had also ITmfDataProviderConfigurationSource in mind, as source of configurations for data providers which would be quite long. However, we already deviated from the initial workflow idea of creating a config first and then applying it. So, a different name might be ok. A configurator/customizer implies that an existing data provider is configure/customized and not new ones are created. I'll think a bit more about it.

abhinava-ericsson commented 1 month ago

The TmfConfigurationSourceManager manages instances of TmfConfigurationSource defined as extensions to the corresponding extension point. Each TmfConfigurationSource has a type TmfConfigurationSourceType that describes the source. Each source is then managing instances of ITmfConfiguration. Hence the term TmfConfigurationManager as replacement to TmfConfigurationTypeManager would not fit, because each source is managing its configurationsm and hence are the manager of configurations. I chose the different name TmfConfigurationTypeManager to distinguish this different difference

Sorry, for 4, I meant to say:

But, if we are sticking with Source for 2 and 3, I guess it doesn't matter.

bhufmann commented 1 month ago

The TmfConfigurationSourceManager manages instances of TmfConfigurationSource defined as extensions to the corresponding extension point. Each TmfConfigurationSource has a type TmfConfigurationSourceType that describes the source. Each source is then managing instances of ITmfConfiguration. Hence the term TmfConfigurationManager as replacement to TmfConfigurationTypeManager would not fit, because each source is managing its configurationsm and hence are the manager of configurations. I chose the different name TmfConfigurationTypeManager to distinguish this different difference

Sorry, for 4, I meant to say:

* Renaming TmfConfigurationSourceManager to TmfConfigurationTypeManager.

But, if we are sticking with Source for 2 and 3, I guess it doesn't matter.

TmfConfigurationTypeManager would make sense as a name. "Source" is not really needed in the class name.

It's already API, so we would adhere to the Trace Compass API policy for breaking changes. However, we could take the stand that these APIs are still provisional (even if not marked as provisional) and it's not used outside Trace Compass Trace Server. In both cases changing it it will require changes in multiple places including TSP that will take some time and effort. I'll discuss with it the team first and see what we are going to do next.

bhufmann commented 1 month ago

I found the "*Source" classes to be some kind of a catch-all which doesn't convey much. I would suggest the following:

  1. Renaming (I)TmfDataProviderSource to (I)TmfDataProviderCustomizer
  2. Renaming (I)TmfConfigurationSourceType to (I)TmfConfigurationType
  3. Renaming (I)TmfConfigurationSource to (I)TmfConfigurationFactory
  4. Renaming TmfConfigurationSourceManager to TmfConfigurationManager

In case 2-4 have already been exposed in Trace Compass API, 1 alone would still be helpful.

I thought about a bit more. For 1) what about (I)TmfDataProviderConfigurator. Customizer for would mean that an existing data provider is customized. @abhinava-ericsson @PatrickTasse WDYT?

Renaming 2-4 would mean a fullstack update (BE, FEs, TSP, Trace Compass), incl. following the API deprecation policy of Trace Compass and this would be quite effort for little gain.

abhinava-ericsson commented 1 month ago

I found the "*Source" classes to be some kind of a catch-all which doesn't convey much. I would suggest the following:

  1. Renaming (I)TmfDataProviderSource to (I)TmfDataProviderCustomizer
  2. Renaming (I)TmfConfigurationSourceType to (I)TmfConfigurationType
  3. Renaming (I)TmfConfigurationSource to (I)TmfConfigurationFactory
  4. Renaming TmfConfigurationSourceManager to TmfConfigurationManager

In case 2-4 have already been exposed in Trace Compass API, 1 alone would still be helpful.

I thought about a bit more. For 1) what about (I)TmfDataProviderConfigurator. Customizer for would mean that an existing data provider is customized. @abhinava-ericsson @PatrickTasse WDYT?

Renaming 2-4 would mean a fullstack update (BE, FEs, TSP, Trace Compass), incl. following the API deprecation policy of Trace Compass and this would be quite effort for little gain.

(I)TmfDataProviderConfigurator looks good to me. Leaving 2-4 as is Ok; just the classes with "Source" in their names should be documented to help a developer parse what "Source" really means in the context of each class.