Skip to content

Improve screen api performance #748

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jul 17, 2023
Merged

Improve screen api performance #748

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
04dcb3c
Improve screen api performance
ryanmelt Jul 16, 2023
59eee77
update specs
ryanmelt Jul 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions openc3-ruby/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ RUN apk update \
ruby-bigdecimal \
ruby-io-console \
ruby-irb \
rbspy \
python3 \
python3-dev \
py3-pip \
Expand Down
24 changes: 4 additions & 20 deletions openc3/lib/openc3/api/limits_api.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,26 +373,10 @@ def _limits_group(group_name, action:, scope:, token:)
# @param item_name [String] item name
# @param scope [String] scope
# @return Hash The requested item based on the packet name
def _get_item(target_name, packet_name, item_name, scope:)
requested_item = nil
if packet_name == 'LATEST'
latest = -1
TargetModel.packets(target_name, scope: scope).each do |packet|
item = packet['items'].find { |item| item['name'] == item_name }
if item
hash = CvtModel.get(target_name: target_name, packet_name: packet['packet_name'], scope: scope)
if hash['PACKET_TIMESECONDS'] && hash['PACKET_TIMESECONDS'] > latest
latest = hash['PACKET_TIMESECONDS']
requested_item = item
end
end
end
raise "Item '#{target_name} LATEST #{item_name}' does not exist" if latest == -1
else
# Determine if this item exists, it will raise appropriate errors if not
requested_item = TargetModel.packet_item(target_name, packet_name, item_name, scope: scope)
end
return requested_item
def _get_item(target_name, packet_name, item_name, cache_timeout: 0.1, scope:)
# Determine if this item exists, it will raise appropriate errors if not
packet_name = CvtModel.determine_latest_packet_for_item(target_name, item_name, cache_timeout: cache_timeout, scope: $openc3_scope) if packet_name == 'LATEST'
return TargetModel.packet_item(target_name, packet_name, item_name, scope: scope)
end
end
end
67 changes: 29 additions & 38 deletions openc3/lib/openc3/api/tlm_api.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,30 +66,30 @@ module Api
# @param args [String|Array<String>] See the description for calling style
# @param type [Symbol] Telemetry type, :RAW, :CONVERTED (default), :FORMATTED, or :WITH_UNITS
# @return [Object] The telemetry value formatted as requested
def tlm(*args, type: :CONVERTED, scope: $openc3_scope, token: $openc3_token)
target_name, packet_name, item_name = tlm_process_args(args, 'tlm', scope: scope)
def tlm(*args, type: :CONVERTED, cache_timeout: 0.1, scope: $openc3_scope, token: $openc3_token)
Copy link
Member

Choose a reason for hiding this comment

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

0.1 is basically just to cache multiple requests in the same cycle but still be responsive to data changing at 10Hz?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's why I chose that as default. It still supports 10Hz, but any faster queries will use the cache.

target_name, packet_name, item_name = tlm_process_args(args, 'tlm', cache_timeout: cache_timeout, scope: scope)
authorize(permission: 'tlm', target_name: target_name, packet_name: packet_name, scope: scope, token: token)
CvtModel.get_item(target_name, packet_name, item_name, type: type.intern, scope: scope)
CvtModel.get_item(target_name, packet_name, item_name, type: type.intern, cache_timeout: cache_timeout, scope: scope)
end

# @deprecated Use tlm with type: :RAW
def tlm_raw(*args, scope: $openc3_scope, token: $openc3_token)
tlm(*args, type: :RAW, scope: scope, token: token)
def tlm_raw(*args, cache_timeout: 0.1, scope: $openc3_scope, token: $openc3_token)
tlm(*args, type: :RAW, cache_timeout: cache_timeout, scope: scope, token: token)
end

# @deprecated Use tlm with type: :FORMATTED
def tlm_formatted(*args, scope: $openc3_scope, token: $openc3_token)
tlm(*args, type: :FORMATTED, scope: scope, token: token)
def tlm_formatted(*args, cache_timeout: 0.1, scope: $openc3_scope, token: $openc3_token)
tlm(*args, type: :FORMATTED, cache_timeout: cache_timeout, scope: scope, token: token)
end

# @deprecated Use tlm with type: :WITH_UNITS
def tlm_with_units(*args, scope: $openc3_scope, token: $openc3_token)
tlm(*args, type: :WITH_UNITS, scope: scope, token: token)
def tlm_with_units(*args, cache_timeout: 0.1, scope: $openc3_scope, token: $openc3_token)
tlm(*args, type: :WITH_UNITS, cache_timeout: cache_timeout, scope: scope, token: token)
end

# @deprecated Use tlm with type:
def tlm_variable(*args, scope: $openc3_scope, token: $openc3_token)
tlm(*args[0..-2], type: args[-1].intern, scope: scope, token: token)
def tlm_variable(*args, cache_timeout: 0.1, scope: $openc3_scope, token: $openc3_token)
tlm(*args[0..-2], type: args[-1].intern, cache_timeout: cache_timeout, scope: scope, token: token)
end

# Set a telemetry item in the current value table.
Expand Down Expand Up @@ -227,16 +227,16 @@ def get_tlm_buffer(target_name, packet_name, scope: $openc3_scope, token: $openc
# @return [Array<String, Object, Symbol|nil>] Returns an Array consisting
# of [item name, item value, item limits state] where the item limits
# state can be one of {OpenC3::Limits::LIMITS_STATES}
def get_tlm_packet(target_name, packet_name, stale_time: 30, type: :CONVERTED, scope: $openc3_scope, token: $openc3_token)
def get_tlm_packet(target_name, packet_name, stale_time: 30, type: :CONVERTED, cache_timeout: 0.1, scope: $openc3_scope, token: $openc3_token)
target_name = target_name.upcase
packet_name = packet_name.upcase
authorize(permission: 'tlm', target_name: target_name, packet_name: packet_name, scope: scope, token: token)
packet = TargetModel.packet(target_name, packet_name, scope: scope)
t = _validate_tlm_type(type)
raise ArgumentError, "Unknown type '#{type}' for #{target_name} #{packet_name}" if t.nil?
items = packet['items'].map { | item | item['name'].upcase }
cvt_items = items.map { | item | "#{target_name}__#{packet_name}__#{item}__#{type}" }
current_values = CvtModel.get_tlm_values(cvt_items, stale_time: stale_time, scope: scope)
cvt_items = items.map { | item | [target_name, packet_name, item, type] }
current_values = CvtModel.get_tlm_values(cvt_items, stale_time: stale_time, cache_timeout: cache_timeout, scope: scope)
Copy link
Member

Choose a reason for hiding this comment

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

Are you changing this method to take an array of array of strings? It used to be an array of strings with the underscores.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this was just an optimization to prevent splitting twice.

Copy link
Member

Choose a reason for hiding this comment

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

Won't that break existing code?

Copy link
Member Author

Choose a reason for hiding this comment

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

If anyone is using CvtModel.get_tlm_values this is a breaking change. That's an internal api though.

items.zip(current_values).map { | item , values | [item, values[0], values[1]]}
end

Expand All @@ -250,25 +250,26 @@ def get_tlm_packet(target_name, packet_name, stale_time: 30, type: :CONVERTED, s
# @return [Array<Object, Symbol>]
# Array consisting of the item value and limits state
# given as symbols such as :RED, :YELLOW, :STALE
def get_tlm_values(items, stale_time: 30, scope: $openc3_scope, token: $openc3_token)
def get_tlm_values(items, stale_time: 30, cache_timeout: 0.1, scope: $openc3_scope, token: $openc3_token)
if !items.is_a?(Array) || !items[0].is_a?(String)
raise ArgumentError, "items must be array of strings: ['TGT__PKT__ITEM__TYPE', ...]"
end
packets = []
cvt_items = []
items.each_with_index do |item, index|
target_name, packet_name, item_name, value_type = item.split('__')
item_upcase = item.to_s.upcase
target_name, packet_name, item_name, value_type = item_upcase.split('__')
raise ArgumentError, "items must be formatted as TGT__PKT__ITEM__TYPE" if target_name.nil? || packet_name.nil? || item_name.nil? || value_type.nil?
target_name = target_name.upcase
packet_name = packet_name.upcase
item_name = item_name.upcase
value_type = value_type.upcase
if packet_name == 'LATEST'
_, packet_name, _ = tlm_process_args([target_name, packet_name, item_name], 'get_tlm_values', scope: scope) # Figure out which packet is LATEST
end
packet_name = CvtModel.determine_latest_packet_for_item(target_name, item_name, cache_timeout: cache_timeout, scope: scope) if packet_name == 'LATEST'
# Change packet_name in case of LATEST and ensure upcase
items[index] = "#{target_name}__#{packet_name}__#{item_name}__#{value_type}"
cvt_items[index] = [target_name, packet_name, item_name, value_type]
packets << [target_name, packet_name]
end
packets.uniq!
packets.each do |target_name, packet_name|
authorize(permission: 'tlm', target_name: target_name, packet_name: packet_name, scope: scope, token: token)
end
CvtModel.get_tlm_values(items, stale_time: stale_time, scope: scope)
CvtModel.get_tlm_values(cvt_items, stale_time: stale_time, cache_timeout: cache_timeout, scope: scope)
end

# Returns an array of all the telemetry packet hashes
Expand Down Expand Up @@ -429,7 +430,7 @@ def _validate_tlm_type(type)
return nil
end

def tlm_process_args(args, method_name, scope: $openc3_scope, token: $openc3_token)
def tlm_process_args(args, method_name, cache_timeout: 0.1, scope: $openc3_scope, token: $openc3_token)
case args.length
when 1
target_name, packet_name, item_name = extract_fields_from_tlm_text(args[0])
Expand All @@ -444,19 +445,9 @@ def tlm_process_args(args, method_name, scope: $openc3_scope, token: $openc3_tok
target_name = target_name.upcase
packet_name = packet_name.upcase
item_name = item_name.upcase

if packet_name == 'LATEST'
latest = -1
TargetModel.packets(target_name, scope: scope).each do |packet|
item = packet['items'].find { |item| item['name'] == item_name }
if item
hash = CvtModel.get(target_name: target_name, packet_name: packet['packet_name'], scope: scope)
if hash['PACKET_TIMESECONDS'] && hash['PACKET_TIMESECONDS'] > latest
latest = hash['PACKET_TIMESECONDS']
packet_name = packet['packet_name']
end
end
end
raise "Item '#{target_name} LATEST #{item_name}' does not exist" if latest == -1
packet_name = CvtModel.determine_latest_packet_for_item(target_name, item_name, cache_timeout: cache_timeout, scope: scope)
else
# Determine if this item exists, it will raise appropriate errors if not
TargetModel.packet_item(target_name, packet_name, item_name, scope: scope)
Expand Down
24 changes: 18 additions & 6 deletions openc3/lib/openc3/models/auth_model.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
# All changes Copyright 2022, OpenC3, Inc.
# All Rights Reserved
#
# This file may also be used under the terms of a commercial license
# This file may also be used under the terms of a commercial license
# if purchased from OpenC3, Inc.

require 'digest'
Expand All @@ -28,6 +28,12 @@ class AuthModel
PRIMARY_KEY = 'OPENC3__TOKEN'
SERVICE_KEY = 'OPENC3__SERVICE__TOKEN'

TOKEN_CACHE_TIMEOUT = 5
Copy link
Member

Choose a reason for hiding this comment

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

I forget all the timeout values ... access token is valid for 5min? This is 5s right? Why this value?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is how long an old password will still work. No affect on Enterprise, only open source.

@@token_cache = nil
@@token_cache_time = nil
@@service_token_cache = nil
@@service_token_cache_time = nil

def self.is_set?(key = PRIMARY_KEY)
Store.exists(key) == 1
end
Expand All @@ -36,15 +42,21 @@ def self.verify(token, permission: nil)
return false if token.nil? or token.empty?

token_hash = hash(token)
return true if Store.get(PRIMARY_KEY) == token_hash
return true if @@token_cache and (Time.now - @@token_cache_time) < TOKEN_CACHE_TIMEOUT and @@token_cache == token_hash
return true if @@service_token_cache and (Time.now - @@service_token_cache_time) < TOKEN_CACHE_TIMEOUT and @@service_token_cache == token_hash and permission != 'admin'

@@token_cache = Store.get(PRIMARY_KEY)
@@token_cache_time = Time.now
return true if @@token_cache == token_hash

service_hash = Store.get(SERVICE_KEY)
if ENV['OPENC3_SERVICE_PASSWORD'] and hash(ENV['OPENC3_SERVICE_PASSWORD']) != service_hash
@@service_token_cache = Store.get(SERVICE_KEY)
@@service_token_cache_time = @@token_cache_time
if ENV['OPENC3_SERVICE_PASSWORD'] and hash(ENV['OPENC3_SERVICE_PASSWORD']) != @@service_token_cache
set_hash = hash(ENV['OPENC3_SERVICE_PASSWORD'])
OpenC3::Store.set(SERVICE_KEY, set_hash)
service_hash = set_hash
@@service_token_cache = set_hash
end
return true if service_hash == token_hash and permission != 'admin'
return true if @@service_token_cache == token_hash and permission != 'admin'
return false
end

Expand Down
Loading